nickdesaulniers abandoned this revision. nickdesaulniers added a comment. In D115409#3190589 <https://reviews.llvm.org/D115409#3190589>, @jyknight wrote:
> OK, I do think that special case definitely needs to be deleted. It's > assuming that the block args are in a particular place in the argument list > of the callbr -- the place Clang put them -- and then assigned special > behavior based on that location. But I think it shouldn't do so -- they > should be able to placed anywhere in the arglist. > > The question then, is why this code needs to exist? It's accomplishing two > things: > > 1. If we get rid of this code but continue to use the "X" constraint, llvm > won't emit an immediate symbol reference (today), rather, it will emit a > register reference instead. Even though X means "accept anything", and > therefore a register is thus supposedly acceptable, it seems like it should > probably prefer emitting an immediate if it can. (GCC does emit it as an > immediate here) Done in https://reviews.llvm.org/D115688. Closing this in favor of that. PTAL. > 2. Emits a TargetBlockAddress instead of BlockAddress. (I'm not sure what the > use of TargetBlockAddress accomplishes? I _think_ it should be okay to use > BlockAddress instead, but I'm not 100% sure...) Not sure about this either way. Perhaps something for reviewers to consider. > Assuming the point 2 is OK, then I think ideally, we'd flip the order of > things, and do: > a. Cause "X" constraint to emit an immediate symbol reference if it can, > rather than copying to a register first -- and remove this special case. > (Fixes bug.) > b. Updates tests and change clang to emit "i" instead of "X" constraints. > (Clarifies intent that an immediate is actually required). Now for me to rebase D115410 <https://reviews.llvm.org/D115410>, D115311 <https://reviews.llvm.org/D115311>, and D115471 <https://reviews.llvm.org/D115471> onto D115688 <https://reviews.llvm.org/D115688>. ================ Comment at: llvm/test/CodeGen/X86/callbr-asm-bb-exports.ll:23 +; CHECK-NEXT: t30: ch,glue = inlineasm_br t22, {{.*}}, t22:1 +; CHECK-NEXT: t32: ch = br t30, BasicBlock:ch ---------------- craig.topper wrote: > nickdesaulniers wrote: > > @craig.topper can you triple check this change to this whole file > > carefully, please? > > > > I'm not sure TBH why my change to SelectionDAGBuilder changed this. I'm > > also not sure of the original intent of the test. It's running > > `-debug-only=isel` which prints A LOT of different phases; I'm not sure > > which it was originally testing. > > > > I'm not sure why we get an `BlockAddress` now; I don't really understand > > the output from selectionDAG here. (`t16` looks unused to me, IIUC). > I think the important thing was that the t22 CopyFromReg chain output is used > by the inlineasm_br. > > I'm guessing the BlockAddress is being created by the `getValue` call in the > else that is now reachable? > > Maybe my regex to ignore most of the inlineasm_br line is hiding too much. Sounds like I should pre-commit (before D115688) a change that unrolls the regex? Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D115409/new/ https://reviews.llvm.org/D115409 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
