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

[Sanjay Patel via Phabricator via llvm-commits]

spatel added a comment.

In D76983#1948546 <https://reviews.llvm.org/D76983#1948546>, @dsprenkels wrote:

> @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?


My git lingo might be off here. Are you saying you don't have commit permissions for LLVM yet?
https://llvm.org/docs/DeveloperPolicy.html#obtaining-commit-access

If not, then yes please create a separate Phab review and someone will push that for you.



================
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;
----------------
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.


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

https://reviews.llvm.org/D76983