[PATCH] D132938: [AMDGPU] Fix crash legalizing G_EXTRACT_VECTOR_ELT with negative index

Thu Sep 8 08:31:10 PDT 2022

arsenm added a comment.

I'd somewhat prefer a mir test for this in case some IR optimization decided to start deleting these earlier

In D132938#3758431 <https://reviews.llvm.org/D132938#3758431>, @Petar.Avramovic wrote:

> IdxVal >= 0 check looks fine to me,
> but what looks wrong in this test is that i1 1(true in llvm ir) index is translated to `%7:_(s32) = G_CONSTANT i32 -1`
> `IRTranslator::translateExtractElement` did not expect i1 indices and since indices are (afaik) positive zext would make more sense then sext in:
> `APInt NewIdx = CI->getValue().sextOrTrunc(PreferredVecIdxWidth);`
>  ->
> `APInt NewIdx = CI->getValue().zextOrTrunc(PreferredVecIdxWidth);`
>
> Language ref does not specify required type for index, although we probably expected i32,
> vector in test `%I` has two elements and i1 is enough to hold index, so globalisel should probably treat `i1` `true` and `false` as `0` and `1` i32 indices .

The type conversion in the IRTranslator surprises me. I would expect to pass through the IR type and let the legalizer deal with it

================
Comment at: llvm/lib/Target/AMDGPU/AMDGPULegalizerInfo.cpp:2346
     return true;
   const int64_t IdxVal = MaybeIdxVal->Value.getSExtValue();

----------------
Why not switch this to getZExtValue?

================
Comment at: llvm/test/CodeGen/AMDGPU/GlobalISel/legalize-extractelement-crash.ll:2
+; NOTE: Assertions have been autogenerated by utils/update_llc_test_checks.py
+; RUN: llc -mtriple=amdgcn -global-isel < %s | FileCheck %s
+
----------------
-global-isel to the front

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D132938