[PATCH] D125774: [InstCombine] fold float trunc into exact itofp for small constants

[Allen zhong via Phabricator via llvm-commits]

Allen added inline comments.


================
Comment at: llvm/lib/Analysis/ValueTracking.cpp:1532
+            // Try to evaluate the value range of the integer.
+            ICmpInst *CmpInst = dyn_cast<ICmpInst>(BO->use_begin()->getUser());
+            if (CmpInst && dyn_cast<ConstantInt>(CmpInst->getOperand(1)) &&
----------------
efriedma wrote:
> Allen wrote:
> > efriedma wrote:
> > > Allen wrote:
> > > > efriedma wrote:
> > > > > I'm very confused by this code.
> > > > > 
> > > > > You're taking the binary operator, checking if the first use in the use list is an icmp, then... assuming the result of the icmp somehow constraints the result of the add?
> > > > > 
> > > > > If you need some sort of reasoning about control flow, maybe look at LazyValueInfo?
> > > > Thanks @efriedma very much. I'll take a look at LazyValueInfo. Since I'm not familiar with this module, more detailed guidance is welcome.
> > > > 
> > > > In current implementation, by using matchSimpleRecurrence, we can know that the initial value of the **%indvar.phi ** is **R**, and the step is **L**. Then, according to the icmp operand, we know that the end value is** KnownEnd**, that is, the current value range is **[KnownEnd. Known2]**. Therefore, if we can know that the most significant n bits of both KnownEnd and Known2 are 0, the most significant n bits of the currently analyzed value **%indvar.phi**  are also necessarily 0.
> > > > ```
> > > > for.body:                                         ; preds = %for.body, %entry
> > > >   %indvar.phi = phi i32 [ 16777215, %entry ], [ %dec.int, %for.body ]
> > > >    ...
> > > >   %dec.int = add nsw i32 %indvar.phi, -1
> > > >   %cmp = icmp ugt i32 %dec.int, 0
> > > >   br i1 %cmp, label %for.body, label %cleanup
> > > > ```
> > > > 
> > > You're missing a bunch of steps here. matchSimpleRecurrence only checks that the PHI has an invariant start, and increases by some "step" each iteration.  It doesn't check that the step is loop-invariant, and it doesn't check that the loop's trip count is actually related to the PHI at all.
> > > 
> > > It might be possible to add the additional checks necessary to make this work here, directly, but it's a lot of complicated code.
> > > 
> > > Instcombine normally doesn't compute LazyValueInfo or ScalarEvolution; a solution involving those probably means moving the transform somewhere else.  Maybe CorrelatedValuePropagation.
> > Thanks @efriedma very much for your detail explanation.
> > I find https://github.com/llvm/llvm-project/blob/main/llvm/lib/Transforms/Scalar/CorrelatedValuePropagation.cpp#L263 is addressing the PHI node, and if I move the transform here,  does the above two check still need or we can use a more simple API to address that ? (make sure step is loop-invariant, loop's trip count is related to the PHI).
> You'd want to compute the actual value range in LazyValueInfo, not CorrelatedValuePropagation itself.  I think LazyValueInfoImpl::solveBlockValuePHINode should do that already for your testcase?  It at least has the right kind of logic.
Thanks @efriedma for your patience.
when I look into the LazyValueInfoImpl::solveBlockValuePHINode (https://github.com/llvm/llvm-project/blob/main/llvm/lib/Analysis/LazyValueInfo.cpp#L721), I find it also analyzes the  PN->getIncomingBlock(i) one by one.
ie, the relationship between the current PHI node and the loop trip is not associated.

Therefore, I directly add the both check on the original basis. (Check the step is loop-invariant, and its loop's trip count is related to current PHI)



================
Comment at: llvm/lib/Transforms/InstCombine/InstCombineCasts.cpp:1757-1758
 
+  // If all the value is within the value range of 2^DestNumSigBits, then the
+  // value can be accurately represented.
+  KnownBits SrcKnown = llvm::computeKnownBits(Src, IC.getDataLayout());
----------------
spatel wrote:
> This part is mentioned in the TODO right under here (update that...) and independent of any control-flow changes, so you could remove the changes to ValueTracking and just focus on this part.
Apply your comment, thanks @spatel 


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

https://reviews.llvm.org/D125774