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

Noah Goldstein via Phabricator via llvm-commits llvm-commits at lists.llvm.org
Wed Jan 4 09:34:38 PST 2023


goldstein.w.n added inline comments.


================
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:
> > 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?
> > 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.
> 
> Oh man...
> 
> **The comment is wrong. We assume `MayChangeDirection` is `false`** (I.e we only find branch if CC matches, not also able to find matches inverse). Sorry for the confusion.
> 
> 
> > 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?
> >
> So at the moment `MayChangeDirection` is unused in both `X86InstrInfo::canMakeTailCallConditional` and `X86InstrInfo::replaceBranchWithTailCall` (assumed **false**). `X86InstrInfo::replaceBranchWithTailCall` needs to find the branch we are replacing which may rely on `MayChangeDirection`. Also we have the sanity tests:
> ```
> assert(canMakeTailCallConditional(BranchCond, TailCall, MayChangeDirection));
> ```
> sitting at the top.
> > 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?
> 
> This is because the entire case is guarded behind:
> ```
>         // Only eliminate if MBB == TBB (Taken Basic Block)
>         if (PredAnalyzable && !PredCond.empty() && PredTBB == MBB &&
>             PredTBB != PredFBB) {
> ```
> So we really are only ever doing this opt if it won't cause CFG reordering already. The `MayChangeDirection` AFAICT is redundant in the current implementation. **Would you prefer me to just turn it into a comment at the call site?**
> 
> Note we do see the final condition switch but from what I can tell that is b.c eliminating this edge in the CFG affects future calls to `BranchFolder::OptimizeBlock`. At the time this transformation takes place it only matches the same taken condition.
> 
> 
> > 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.
> 
> Oh man...
> 
> **The comment is wrong. We assume `MayChangeDirection` is `false`** (I.e we only find branch if CC matches, not also able to find matches inverse). Sorry for the confusion.
> 

I fixed the wrong comment in V2. Let me know your decision about dropping `MayChangeDirection` entirely for V3.
> 
> > 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?
> >
> So at the moment `MayChangeDirection` is unused in both `X86InstrInfo::canMakeTailCallConditional` and `X86InstrInfo::replaceBranchWithTailCall` (assumed **false**). `X86InstrInfo::replaceBranchWithTailCall` needs to find the branch we are replacing which may rely on `MayChangeDirection`. Also we have the sanity tests:
> ```
> assert(canMakeTailCallConditional(BranchCond, TailCall, MayChangeDirection));
> ```
> sitting at the top.
> > 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?
> 
> This is because the entire case is guarded behind:
> ```
>         // Only eliminate if MBB == TBB (Taken Basic Block)
>         if (PredAnalyzable && !PredCond.empty() && PredTBB == MBB &&
>             PredTBB != PredFBB) {
> ```
> So we really are only ever doing this opt if it won't cause CFG reordering already. The `MayChangeDirection` AFAICT is redundant in the current implementation. **Would you prefer me to just turn it into a comment at the call site?**
> 
> Note we do see the final condition switch but from what I can tell that is b.c eliminating this edge in the CFG affects future calls to `BranchFolder::OptimizeBlock`. At the time this transformation takes place it only matches the same taken condition.
> 
> 




Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D140931



More information about the llvm-commits mailing list