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

[Serguei Katkov via Phabricator via llvm-commits]

skatkov added inline comments.


================
Comment at: llvm/lib/CodeGen/BranchFolding.cpp:1684
+      if (CurTBB && IsBranchOnlyBlock(MBB) &&
+          AreOperandsIdentical(CurCond, PriorCond) && !MBB->hasAddressTaken() &&
+          !MBB->isEHPad()) {
----------------
goldstein.w.n wrote:
> 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.
your question still makes sense. If on some platform there exist instruction which does a comparison and jump (for example, cbz on AARCH) I need to ensure that there is no modification of memory/register between two such operations...

In this patch I'm base on analyzeBranch utility. And It might be a problem. I'll try to create some tests which reproduce this problem and probably I need to find how to fix it.
Moreover if it is true problem then probably analyzeBranch is not enough and I need to do something more and probably in different place.


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

https://reviews.llvm.org/D148514