[PATCH] D139943: [CodeGen][AMDGPU] EXTRACT_VECTOR_ELT: input vector element type can differ from output type

[Juan Manuel Martinez Caamaño via Phabricator via llvm-commits]

jmmartinez created this revision.
Herald added subscribers: kosarev, foad, kerbowa, hiraditya, tpr, dstuttard, yaxunl, jvesely, kzhuravl, arsenm.
Herald added a project: All.
arsenm added a comment.
jmmartinez updated this revision to Diff 482735.
jmmartinez updated this revision to Diff 484200.
jmmartinez published this revision for review.
jmmartinez added a reviewer: arsenm.
Herald added subscribers: llvm-commits, wdng.
Herald added a project: LLVM.

Implicit element conversion strikes again. LGTM but needs tests


jmmartinez added a comment.

Corrected binop case

Still missing tests for all of this


jmmartinez added a comment.

In D139943#3992295 <https://reviews.llvm.org/D139943#3992295>, @arsenm wrote:

> Implicit element conversion strikes again. LGTM but needs tests

Sure sorry! I didn't know phabricatior sent notifications when the patch is marked as draft. I submitted it in draft mode and I hoped to submit it after adding the tests.


jmmartinez added a comment.

- Added test (I found the test a bit awkward but I haven't managed to trigger it otherwise)



================
Comment at: llvm/lib/Target/AMDGPU/SIISelLowering.cpp:10814
   // ScalarRes = scalar-BINOP Vec1Elt, Vec2Elt
   if (Vec.hasOneUse() && DCI.isBeforeLegalize()) {
     SDLoc SL(N);
----------------
This condition has to be stengtened. This is valid for smin, smax, umin and umax if the types have the same size or if they are truncated. 


================
Comment at: llvm/lib/Target/AMDGPU/SIISelLowering.cpp:10814
   // ScalarRes = scalar-BINOP Vec1Elt, Vec2Elt
   if (Vec.hasOneUse() && DCI.isBeforeLegalize()) {
     SDLoc SL(N);
----------------
jmmartinez wrote:
> This condition has to be stengtened. This is valid for smin, smax, umin and umax if the types have the same size or if they are truncated. 
Not sure what you mean, those are handled below?


================
Comment at: llvm/lib/Target/AMDGPU/SIISelLowering.cpp:10814
   // ScalarRes = scalar-BINOP Vec1Elt, Vec2Elt
   if (Vec.hasOneUse() && DCI.isBeforeLegalize()) {
     SDLoc SL(N);
----------------
arsenm wrote:
> jmmartinez wrote:
> > This condition has to be stengtened. This is valid for smin, smax, umin and umax if the types have the same size or if they are truncated. 
> Not sure what you mean, those are handled below?
I meant that when `ResVT != VecEltVT` the transformation could not be valid.

For example, if `ResVT == i8` but `VecEltVT == i16` for the min/max operations.

I wasn't able to trigger this path, so I haven't added the code to do the transformation. Still, I added a check in the `if` anyways.


================
Comment at: llvm/test/CodeGen/AMDGPU/dagcomb-extract-vec-elt-different-sizes.ll:1
+; NOTE: Assertions have been autogenerated by utils/update_llc_test_checks.py
+; RUN: llc -mtriple=amdgcn-amd-amdhsa -mcpu=gfx90a -verify-machineinstrs < %s | FileCheck -enable-var-scope %s
----------------
@arsenm If you have some advice about how to reduce even more this test case I'll be glad.

I tried triggering this case by hand but I didn't succeed. In the end I used `llvm-reduce` and applied some optimizations to get up to this.


In function SITargetLowering::performExtractVectorElt,
the output type was not considered which could lead to type mismatches
later.


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D139943

Files:
  llvm/lib/Target/AMDGPU/SIISelLowering.cpp
  llvm/test/CodeGen/AMDGPU/dagcomb-extract-vec-elt-different-sizes.ll

-------------- next part --------------
A non-text attachment was scrubbed...
Name: D139943.484200.patch
Type: text/x-patch
Size: 10481 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20221220/24447ca4/attachment.bin>