[PATCH] D54827: [simplifycfg] Handle 3 sequential branches while the first two can infer the third one.

[luo xionghu via Phabricator via llvm-commits]

yinyuefengyi added inline comments.


================
Comment at: llvm/lib/Transforms/Utils/SimplifyCFG.cpp:5953-5955
+          Value *CombinedAndCmp = GetCombinedCompareInstruction(
+              Builder, PPBI->getCondition(), PBI->getCondition(), PCondIsTrue,
+              CondIsTrue);
----------------
spatel wrote:
> yinyuefengyi wrote:
> > spatel wrote:
> > > lebedev.ri wrote:
> > > > yinyuefengyi wrote:
> > > > > lebedev.ri wrote:
> > > > > > And this too might already exist..
> > > > > > Have you seen [[ https://github.com/llvm-mirror/llvm/blob/5e3c9a56cb080c7a5131d6063d04d1d4a22044d0/lib/Transforms/InstCombine/InstCombineAndOrXor.cpp | `InstCombiner::foldAndOfICmps()` ]] ?
> > > > > Thanks for the comments.
> > > > > I've read the InstCombiner::foldAndOfIcmps(),  it is a local member functions with a lot of other member functions called inside, seems not easy to call it directly from class SimplifyCFGOpt, do you have any suggestions about this?
> > > > Perhaps it can be promoted to not be `private`? (cc @spatel)
> > > > https://github.com/llvm-mirror/llvm/blob/5e3c9a56cb080c7a5131d6063d04d1d4a22044d0/lib/Transforms/InstCombine/InstCombineInternal.h#L437-L581
> > > > Duplicating the exact same logic is probably not the best answer here..
> > > I think the way to share the code is to move it from InstCombine to CmpInstAnalysis.cpp. That's how CmpInstAnalysis was created in the first place.
> > > 
> > > I'm not sure if would avoid that work, but have you looked at trying to simplify the icmp in CorrelatedValuePropagation instead of here in SimplifyCFG?
> > Thanks for the reply.
> > This simplifyCFG patch is cross basic block optimization that need back access 3 block's prediction instruction, no dependency of other passes.  But the CorrelatedValuePropagation can only do instruction level optimization IN BLOCK based on LazyValueInfo, like infer the condition "x < Const" to be true or false, quite different with this patch's target.
> > 
> > Also, I've investigated about moving InstCombiner::foldAndOfICmps() to other file like CmpInstAnalysis.cpp, more than 1000+ LOC changes required as this function calls 10+ member functions like foldAndOrOfICmpsOfAndWithPow2(),  simplifyRangeCheck(), insertRangeTest() and static functions  getNewICmpValue(), foldLogOpOfMaskedICmps,foldLogOpOfMaskedICmps(),  foldAndOrOfEqualityCmpsWithConstants(), foldSignedTruncationCheck(). In addition, the variable IRBuilder<TargetFolder, IRBuilderCallbackInserter> is different from outside need change.
> > Seems this HUGE refactor should be done in another patch if necessary, after the foldAndOfICmps() is moved to accessible place, this SimplifyCFGOpt patch can be tidy.
> > 
> Thanks for checking on CVP. I haven't looked into that pass very much, but if you can simplify the icmp to true/false within that pass, then doesn't that allow dead code elimination/instcombine/simplifycfg to later kill all of the unused instructions?
> 
> I agree that any refactoring of the code from instcombine should be done in separate patches from this one. I also agree that it's a big mess of code. Maybe we can break that job up to allow progress for this patch?
> 
> As a preliminary step, you can commit the test cases from this patch to trunk with the current output. For that, please use FileCheck rather than 'grep' and use utils/update_test_checks.py to generate the assertions. There are many examples of test files that use that script, but feel free to ask questions if it's not clear how to use it. I also ask that you give each test a comment, so we can tell what the differences are between the tests.
Thanks, spatel. The test case is already split and uploaded to https://reviews.llvm.org/D54994.

The problem is this type of icmps ( a>=b && a!=b  =>  a>b) can't be simplified, they can only be folded by the function InstCombiner::foldAndOfICmps().

I just come up with another idea that whether can this be implemented in this way of combination optimization:
opt < %s -simplifycfg -instcombine -simplifycfg -S

the first round of 'simplifycfg' will do the condition check , if BCmpPred equals to (ACmpPred && BCmpPred), do nothing(means it may be folded, to avoid endless loop), else replace the BCmpPred with the AND instruction, this AND instruction can be folded to BCmpPred2(i.e. a>b) by the next 'instcombine' pass in foldAndOfICmps(). then the second round of 'simplifycfg' can do the implication check with BCmpPred2 && CCmpPred, to merge Block 3 or eliminated it.

I tried this experiment on my code base, it can work. But I am not sure this kind of pass combination introduces PASS DEPENDENCY will cause side effect or break the LLVM design rule. If this implementation is also not acceptable, then possibly the only way is refactoring the function foldAndOfICmps()  from instcombine. Fundamentally, a shared foldAndOfICmps() can achieve best benefits.



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

https://reviews.llvm.org/D54827