[PATCH] D115409: [SelectionDAGBuilder] drop special handling for CallBr

Tue Dec 14 16:21:06 PST 2021

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