Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

[IrTranslator] Sign extends index value for G_EXTRACT_VECTOR_ELT, translating i1 true into i32 -1. #57452

Closed
DataCorrupted opened this issue Aug 30, 2022 · 2 comments
Closed

[IrTranslator] Sign extends index value for G_EXTRACT_VECTOR_ELT, translating i1 true into i32 -1. #57452

DataCorrupted opened this issue Aug 30, 2022 · 2 comments
Labels
llvm:core

Comments

@DataCorrupted
Copy link
Member

DataCorrupted commented Aug 30, 2022 •
edited by VoltrexKeyva
Loading

This is an issue related to #57408
Given the following seed and command. Or you can see the result here: https://llvm.godbolt.org/z/dd5edjrG3

llc PoC.ll -stop-after=irtranslator  -global-isel -o PoC.mir
define void @f() {
BB:
  br label %BB2

BB2:                                              ; preds = %BB
  br label %BB1

BB1:                                              ; preds = %BB2
  %B1 = sub <2 x i8> <i8 16, i8 16>, <i8 16, i8 16>
  %A = alloca i8, align 4
  %L1 = load i8, i8* %A, align 1
  %I = insertelement <2 x i8> %B1, i8 -128, i8 %L1
  %E1 = extractelement <2 x i8> %I, i1 true
  store i8 %E1, i8* %A, align 1
  ret void
}

Regardless the architecture, IrTranslator will translate the index i1 true into %9:_(s64) = G_CONSTANT i64 -1 (or s32 -1 on 32bit architecture).

This has caused AMDGPU backend to crash since negative index is unexpected. (See #57408 )

Further study shows that we are sign extending the index at IRTranslator.cpp:2811, thus given values like i1 true, i8 255, the index will be translated as -1.

We propose this change

- APInt NewIdx = CI->getValue().sextOrTrunc(PreferredVecIdxWidth);
+ APInt NewIdx = CI->getValue().zextOrTrunc(PreferredVecIdxWidth);

as mentioned by @petar-avramovic in the latest review: https://reviews.llvm.org/D132938
@DataCorrupted
Copy link
Member Author

See patch: https://reviews.llvm.org/D132978

DataCorrupted added a commit that referenced this issue Oct 15, 2022
@DataCorrupted
[CodeGen] Using ZExt for extractelement indices.
c2e7c9c 
In #57452, we found that IRTranslator is translating `i1 true` into `i32 -1`.
This is because IRTranslator uses SExt for indices.

In this fix, we change the expected behavior of extractelement's index, moving from SExt to ZExt.
This change includes both documentation, SelectionDAG and IRTranslator.
We also included a test for AMDGPU, updated tests for AArch64, Mips, PowerPC, RISCV, VE, WebAssembly and X86

This patch fixes issue #57452.

Differential Revision: https://reviews.llvm.org/D132978
@DataCorrupted
Copy link
Member Author

DataCorrupted commented Oct 17, 2022 •
edited
Loading

Has been fixed and merged in commit c2e7c9cb33ac

sid8123 pushed a commit to sid8123/llvm-project that referenced this issue Oct 25, 2022
@DataCorrupted
[CodeGen] Using ZExt for extractelement indices.
1838391 
In llvm#57452, we found that IRTranslator is translating `i1 true` into `i32 -1`.
This is because IRTranslator uses SExt for indices.

In this fix, we change the expected behavior of extractelement's index, moving from SExt to ZExt.
This change includes both documentation, SelectionDAG and IRTranslator.
We also included a test for AMDGPU, updated tests for AArch64, Mips, PowerPC, RISCV, VE, WebAssembly and X86

This patch fixes issue llvm#57452.

Differential Revision: https://reviews.llvm.org/D132978
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
llvm:core
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
2 participants
@EugeneZelenko @DataCorrupted