[PATCH] D154717: [LV] Check if ops can safely be truncated in computeMinimumValueSizes.

[Florian Hahn via Phabricator via llvm-commits]

fhahn marked an inline comment as done.
fhahn added inline comments.


================
Comment at: llvm/lib/Analysis/VectorUtils.cpp:678
+              if (I.trunc(MinBW).zext(I.getBitWidth()) == I)
+                return false;
+            }
----------------
nikic wrote:
> fhahn wrote:
> > nikic wrote:
> > > fhahn wrote:
> > > > nikic wrote:
> > > > > Hm, is this special case for constants correct? Consider a variant on the test case that does something like `%l = shl i32 %f, 24`. Then only the low 8 bits of `%f` would be demanded, and `24` can be truncated to 8 bits, but it still changes the result: Now the shift would return poison (while previously the shl + trunc would produce zero).
> > > > Yep, it seems like there are multiple issues with this code. For the `lshr` case, DemandedBits seems sufficient to rule out narrowing (by making sure we don't drop bits contributing to the result).
> > > > 
> > > > In addition to that for the `shl` case (and others where smaller widths may cause new poison), we would also need to take into account the semantics of the specific instructions. Do we have any existing APIs that would be suitable to check?
> > > I think if we only use the demanded bits check below, that should always be correct. Would it be sufficient to only have the constant special case for shift amounts (with the appropriate bit width check)? That's probably the main case where we don't get meaningful demanded bits on one of the operands.
> > Updated to check the constant shift amount. Was that what you had in mind?
> Not quite. This is the code I would expect:
> ```
>       if (any_of(MI->operands(), [&DB, MinBW](Use &U) {
>             auto *CI = dyn_cast<ConstantInt>(U); 
>             if (isa<ShlOperator, LShrOperator, AShrOperator>(U.getUser()) &&
>                 U.getOperandNo() == 1 && CI) 
>               return CI->getValue().uge(MinBW);
>                
>             uint64_t BW = bit_width(DB.getDemandedBits(&U).getZExtValue());
>             return bit_ceil(BW) > MinBW;
>           }))
>         continue;
> ```
> That is, use demanded bits for everything apart from shift amounts, including all other constant operands. That way it's always conservatively correct.
Sounds good to me! Updated, thanks.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D154717