[PATCH] D140931: Improve and enable folding of conditional branches with tail calls.

[Phoebe Wang via Phabricator via llvm-commits]

pengfei added a comment.

In D140931#4025158 <https://reviews.llvm.org/D140931#4025158>, @goldstein.w.n wrote:

> In D140931#4025075 <https://reviews.llvm.org/D140931#4025075>, @pengfei wrote:
>
>> I'm not sure if we can arbitrarily replace `jmp foo` with `jcc foo`. At least `jmp` has a large range (64-bit) scope which supports code size > 2GB. Maybe we should limit it to kernel/small code model?
>
> Can it? https://www.felixcloutier.com/x86/jmp I though `rel32` was maximum imm encoding.
>
> But in `canMakeTailCallConditional` we have:
>
>   if (TailCall.getOpcode() != X86::TCRETURNdi &&
>       TailCall.getOpcode() != X86::TCRETURNdi64) {
>     // Only direct calls can be done with a conditional branch.
>     return false;
>   }
>
> We could limit `X86::TCRETURNdi64` with kernel/small-code model. ~~Although this is done after
> reg alloc and only within function so we could only do if function has less than 2^32 / 16 instructions
> and be safe.~~
>
> Edit:
> The stuff about in function is wrong.

I didn't look into the details in `canMakeTailCallConditional`. Seems we have checked for several cases. So this is not a fresh question, or maybe even not a problem. At least, it's not specific to this patch.



================
Comment at: llvm/lib/Target/X86/X86InstrInfo.cpp:3018-3019
     assert(BranchCond.size() == 1);
+    // At the moment MayChangeDirection is unused (or always treated as true).
+    // We only match existing CC.
     if (CC != BranchCond[0].getImm())
----------------
goldstein.w.n wrote:
> pengfei wrote:
> > We do we need to add it if it's unused? Do not put unrelated/useless code in the same patch. It obstructs people to understand the patch.
> Its not needed. The reason I added it was b.c the rationale previously for keeping this optimization guarded behind `-Os` was that if it changed branch direction + reordered CFG it could be a de-optimization. That seems like a reasonable concern so thought it would be good to have in the generic API.
> 
> A more appropriate comment might have been: "At the moment never change branch direction".
> 
> Can drop if your prefer. Let me know for V2.
I'm still confused by the intention of this flag. If you believe it's a reasonable concern, why can we do it by default in this patch? The flag doesn't prevent from doing it.
OTOH, if we want make target specific decision, should we simply add the code
```
if (MayChangeDirection)
  return false;
```
to other targets (if they override it)? So we don't need to pass `MayChangeDirection` again to `replaceBranchWithTailCall`, right?

Regarding de-optimization, I had the same concern at the beginning. But test cases show that in most cases both branches are terminators, either jmp or ret. So the change of the layout should have negligible affect to performance?


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D140931/new/

https://reviews.llvm.org/D140931