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

[Sanjay Patel via Phabricator via llvm-commits]

spatel added inline comments.


================
Comment at: llvm/lib/Transforms/Utils/SimplifyCFG.cpp:5953-5955
+          Value *CombinedAndCmp = GetCombinedCompareInstruction(
+              Builder, PPBI->getCondition(), PBI->getCondition(), PCondIsTrue,
+              CondIsTrue);
----------------
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.


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

https://reviews.llvm.org/D54827