[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:
> 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.



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

https://reviews.llvm.org/D54827