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

[Daan Sprenkels via Phabricator via llvm-commits]

dsprenkels marked an inline comment as done.
dsprenkels added a comment.

@spatel Thanks for the review. I will soon look into it!

> Generate the baseline CHECK lines without this code patch in place and push the tests to master as a preliminary NFC patch. Then apply this code patch, so we see the code diffs resulting from this patch in this review. That way, the tests will safely remain even if this code patch has to be reverted for some reason.

I don't think I have the permissions do push directly to master. Is this easily fixed? Or should I create a separate differential for this?



================
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:
> 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?


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

https://reviews.llvm.org/D76983