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


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

https://reviews.llvm.org/D125774