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

[Eli Friedman via Phabricator via llvm-commits]

efriedma 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)) &&
----------------
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.


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

https://reviews.llvm.org/D125774