[PATCH] D76983: [InstCombine] Transform extractelement-trunc -> bitcast-extractelement

[Daan Sprenkels via Phabricator via llvm-commits]

dsprenkels marked 5 inline comments as done.
dsprenkels added a comment.

> My git lingo might be off here. Are you saying you don't have commit permissions for LLVM yet?

Yup. Should I ask for it? (I don't know what the etiquette is; i.e. how many patches before one should request commit access.)



================
Comment at: llvm/lib/Transforms/InstCombine/InstCombineCasts.cpp:855
+  ///   extractelement <8 x i32> (bitcast <4 x i64> %X to <8 x i32>), i32 0
+  Value *VecOp = nullptr;
+  if (match(Src,
----------------
spatel wrote:
> Don't initialize this to nullptr. If the match fails, the variable should not be used, so initializing hides a potential compile-time warning for an unintended use of that variable. The same should be true of the existing variables, so if you want to fix them 1st with an NFC patch, that would be ok.
I updated every variable that this patch touches. I can probably fix the others later in an NFC patch.


================
Comment at: llvm/lib/Transforms/InstCombine/InstCombineCasts.cpp:864
+    // A badly fit destination size would result in an invalid cast.
+    if (VecNumElts * VecOpScalarSize % DestScalarSize == 0) {
+      unsigned BitCastNumElts = VecNumElts * VecOpScalarSize / DestScalarSize;
----------------
spatel wrote:
> dsprenkels wrote:
> > spatel wrote:
> > > The VecNumElts factor doesn't change the modulo constraint?
> > This is intentional. In this check, I need the bit-width of the whole vector. Consider this example:
> > 
> > <http://volta.cs.utah.edu:8080/z/-GpGHX>
> > 
> >   target datalayout = "e"
> >   
> >   define i30 @src(<3 x i40> %x) {
> >     %e = extractelement <3 x i40> %x, i32 0
> >     %t = trunc i40 %e to i30
> >     ret i30 %t
> >   }
> >   
> >   define i30 @tgt(<3 x i40> %x) {
> >     %1 = bitcast <3 x i40> %x to <4 x i30>
> >     %t = extractelement <4 x i30> %1, i30 0
> >     ret i30 %t
> >   }
> > 
> > This describes a valid case to be canonicalized by this patch, however `VecOpScalarSize % DestScalarSize == 40 % 30 != 0`. That is why I check if `(VecNumElts * VecOpScalarSize) % DestScalarSize == (3 * 40) % 30 == 0`.
> > 
> > Should I add a comment here to clarify this?
> Hmm...how does that example translate for big-endian?
> A code comment is good; another regression test is even better.
You are right, this case would lead to an invalid transform. I fixed this, and added a test in <https://reviews.llvm.org/D77024> that checks that these cases should not be changed.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D76983