[PATCH] D148514: [BranchFolding] Remove redundant conditional branch.

[Noah Goldstein via Phabricator via llvm-commits]

goldstein.w.n added inline comments.


================
Comment at: llvm/lib/CodeGen/BranchFolding.cpp:1684
+      if (CurTBB && IsBranchOnlyBlock(MBB) &&
+          AreOperandsIdentical(CurCond, PriorCond) && !MBB->hasAddressTaken() &&
+          !MBB->isEHPad()) {
----------------
skatkov wrote:
> skatkov wrote:
> > goldstein.w.n wrote:
> > > skatkov wrote:
> > > > skatkov wrote:
> > > > > goldstein.w.n wrote:
> > > > > > Does this take into account comparisons against memory? The pointer (operand) stays the same but value in memory is modified in BB1?
> > > > > BB1 is a predecessor or MBB? For MBB we check that it is IsBranchOnlyBlock(MBB).
> > > > > 
> > > > > you mean that predecessor may contains store AFTER conditional branch and conditional branch itself may reference memory?
> > > > > 
> > > > and -> or
> > > > BB1 is a predecessor or MBB? For MBB we check that it is IsBranchOnlyBlock(MBB).
> > > > 
> > > > you mean that predecessor may contains store AFTER conditional branch and conditional branch itself may reference memory?
> > > > 
> > > Yes, exactly.
> > > If this is still in SSA then it's fine, but x86 machine instructions (and other) have `rm` modifier where operands match but results can vary
> > > 
> > Interesting, something new to me. At least on x86_64 conditional instruction is JCC which actually only reads the eflags and has no access to memory. If there is no other modification of eflags - we are ok.
> > 
> > But at least I need to check two things:
> > 1) is it possible that there are some instructions after conditional branch which may affect the result of next equivalent conditional branch.
> > 2) Are there any platforms where two conditional branches, one right after another one, may produce the right results.
> > 
> > Thanks for good question.
> may produce the right results. -> may produce different results.
> Interesting, something new to me. At least on x86_64 conditional instruction is JCC which actually only reads the eflags and has no access to memory. If there is no other modification of eflags - we are ok.
> 
> But at least I need to check two things:
> 1) is it possible that there are some instructions after conditional branch which may affect the result of next equivalent conditional branch.
> 2) Are there any platforms where two conditional branches, one right after another one, may produce the right results.
> 
> Thanks for good question.

I think I confused myself (and possibly you a bit).

I assumed match operands was matching the comparison that lead to it i.e

`cmpl %rdi, (%rsi); jcc` not just the `jcc`. If its just the `jcc` then yeah, there is no concern about the  `rm` stuff.

If its just `jcc`, then yeah just checking that there are no instructions that write to the jump flags should be enough.


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

https://reviews.llvm.org/D148514