[PATCH] D66057: [InstCombine] Shift amount reassociation in bittest: trunc-of-shl (PR42399)

[Sanjay Patel via Phabricator via llvm-commits]

spatel accepted this revision.
spatel added a comment.
This revision is now accepted and ready to land.

LGTM



================
Comment at: llvm/test/Transforms/InstCombine/shift-amount-reassociation-in-bittest-with-truncation-shl.ll:414
   %t1 = lshr i32 %x, %t0
-  %t2 = add i32 %len, 1 ; too much
+  %t2 = add i32 %len, 32 ; too much
   %t2_wide = zext i32 %t2 to i64
----------------
lebedev.ri wrote:
> spatel wrote:
> > Please explain and/or add a comment here.
> > If we leave the add constant as 1, this gets simplified away completely with this patch? Why doesn't it happen with 32? 
> > Do we already have a test that covers the case for the original test (with add of 1)?
> With `%t2 = add i32 %len, 1` the total shift is `((32-len)+(len+1))`==`33`.
> The widest type here is `i64`, and 33 is a valid shift amount there.
> But since one of the values being shifted is i32, the result can be constant-folded to false
> since you will effectively shift-out all the bits.
> https://rise4fun.com/Alive/kk1
> 
> With 32 here, the total shift amount will be 64, which is not okay for i64.
> But that would already imply UB, and we **should** be folding it into `undef`
> https://rise4fun.com/Alive/Tzn
> 
> I'm trying to keep changes reviewable, so i did not get to that part yet.
> 
> The `@t9_oneuse5` gets constant-folded too, so there is some coverage.
Ok - thanks for the explanation.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D66057