[TypePromotion] Do not zero-extend getelementptr indexes since signed #134281
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Member
|
@llvm/pr-subscribers-llvm-transforms @llvm/pr-subscribers-backend-aarch64 Author: Antonio Frighetto (antoniofrighetto) ChangesA miscompilation issue has been addressed with improved handling. Fixes: #133928. Alive2 for signext_bitcast_phi_select test: https://alive2.llvm.org/ce/z/gcMNvS. Full diff: https://github.com/llvm/llvm-project/pull/134281.diff 4 Files Affected:
diff --git a/llvm/lib/CodeGen/TypePromotion.cpp b/llvm/lib/CodeGen/TypePromotion.cpp
index b1f99094daa4a..01db72cbb8e85 100644
--- a/llvm/lib/CodeGen/TypePromotion.cpp
+++ b/llvm/lib/CodeGen/TypePromotion.cpp
@@ -809,7 +809,7 @@ bool TypePromotionImpl::TryToPromote(Value *V, unsigned PromotedWidth,
// Ignore GEPs because they don't need promoting and the constant indices
// will prevent the transformation.
if (isa<GetElementPtrInst>(V))
- return true;
+ return false;
if (!isSupportedValue(V) || (shouldPromote(V) && !isLegalToPromote(V))) {
LLVM_DEBUG(dbgs() << "IR Promotion: Can't handle: " << *V << "\n");
diff --git a/llvm/test/CodeGen/AArch64/typepromotion-phisret.ll b/llvm/test/CodeGen/AArch64/typepromotion-phisret.ll
index d60578b7bafe4..a9164312b99cc 100644
--- a/llvm/test/CodeGen/AArch64/typepromotion-phisret.ll
+++ b/llvm/test/CodeGen/AArch64/typepromotion-phisret.ll
@@ -237,25 +237,27 @@ define i16 @promote_arg_return(i16 zeroext %arg1, i16 zeroext %arg2, ptr %res) {
define i16 @signext_bitcast_phi_select(i16 signext %start, ptr %in) {
; CHECK-LABEL: signext_bitcast_phi_select:
; CHECK: // %bb.0: // %entry
-; CHECK-NEXT: mov w8, #-1 // =0xffffffff
-; CHECK-NEXT: and w9, w0, #0xffff
-; CHECK-NEXT: cmp w8, w9, sxth
+; CHECK-NEXT: mov w9, #-1 // =0xffffffff
+; CHECK-NEXT: mov w10, #32768 // =0x8000
+; CHECK-NEXT: // kill: def $w0 killed $w0 def $x0
+; CHECK-NEXT: cmp w9, w0, sxth
; CHECK-NEXT: b.lt .LBB6_3
; CHECK-NEXT: .LBB6_1: // %if.then
; CHECK-NEXT: // =>This Inner Loop Header: Depth=1
-; CHECK-NEXT: ldrh w0, [x1, w9, sxtw #1]
-; CHECK-NEXT: cmp w0, w9
+; CHECK-NEXT: sxth x8, w0
+; CHECK-NEXT: ldrh w8, [x1, x8, lsl #1]
+; CHECK-NEXT: cmp w8, w0, uxth
; CHECK-NEXT: b.eq .LBB6_4
; CHECK-NEXT: // %bb.2: // %if.else
; CHECK-NEXT: // in Loop: Header=BB6_1 Depth=1
-; CHECK-NEXT: lsr w10, w9, #15
-; CHECK-NEXT: eor w10, w10, #0x1
-; CHECK-NEXT: add w9, w10, w9
-; CHECK-NEXT: cmp w8, w9, sxth
+; CHECK-NEXT: bic w8, w10, w0
+; CHECK-NEXT: add w0, w0, w8, lsr #15
+; CHECK-NEXT: cmp w9, w0, sxth
; CHECK-NEXT: b.ge .LBB6_1
; CHECK-NEXT: .LBB6_3:
-; CHECK-NEXT: mov w0, wzr
+; CHECK-NEXT: mov w8, wzr
; CHECK-NEXT: .LBB6_4: // %exit
+; CHECK-NEXT: mov w0, w8
; CHECK-NEXT: ret
entry:
%const = bitcast i16 -1 to i16
diff --git a/llvm/test/Transforms/TypePromotion/ARM/phis-ret.ll b/llvm/test/Transforms/TypePromotion/ARM/phis-ret.ll
index 6f41742e66e53..d3ba82460a18a 100644
--- a/llvm/test/Transforms/TypePromotion/ARM/phis-ret.ll
+++ b/llvm/test/Transforms/TypePromotion/ARM/phis-ret.ll
@@ -293,28 +293,24 @@ define i16 @promote_arg_return(i16 zeroext %arg1, i16 zeroext %arg2, ptr %res) {
define i16 @signext_bitcast_phi_select(i16 signext %start, ptr %in) {
; CHECK-LABEL: @signext_bitcast_phi_select(
; CHECK-NEXT: entry:
-; CHECK-NEXT: [[TMP0:%.*]] = zext i16 [[START:%.*]] to i32
; CHECK-NEXT: [[CONST:%.*]] = bitcast i16 -1 to i16
; CHECK-NEXT: br label [[FOR_BODY:%.*]]
; CHECK: for.body:
-; CHECK-NEXT: [[IDX:%.*]] = phi i32 [ [[SELECT:%.*]], [[IF_ELSE:%.*]] ], [ [[TMP0]], [[ENTRY:%.*]] ]
-; CHECK-NEXT: [[TMP1:%.*]] = trunc i32 [[IDX]] to i16
+; CHECK-NEXT: [[TMP1:%.*]] = phi i16 [ [[SELECT:%.*]], [[IF_ELSE:%.*]] ], [ [[START:%.*]], [[ENTRY:%.*]] ]
; CHECK-NEXT: [[CMP_I:%.*]] = icmp sgt i16 [[TMP1]], [[CONST]]
; CHECK-NEXT: br i1 [[CMP_I]], label [[EXIT:%.*]], label [[IF_THEN:%.*]]
; CHECK: if.then:
-; CHECK-NEXT: [[IDX_NEXT:%.*]] = getelementptr i16, ptr [[IN:%.*]], i32 [[IDX]]
+; CHECK-NEXT: [[IDX_NEXT:%.*]] = getelementptr i16, ptr [[IN:%.*]], i16 [[TMP1]]
; CHECK-NEXT: [[LD:%.*]] = load i16, ptr [[IDX_NEXT]], align 2
-; CHECK-NEXT: [[TMP2:%.*]] = zext i16 [[LD]] to i32
-; CHECK-NEXT: [[CMP1_I:%.*]] = icmp eq i32 [[TMP2]], [[IDX]]
+; CHECK-NEXT: [[CMP1_I:%.*]] = icmp eq i16 [[LD]], [[TMP1]]
; CHECK-NEXT: br i1 [[CMP1_I]], label [[EXIT]], label [[IF_ELSE]]
; CHECK: if.else:
-; CHECK-NEXT: [[LOBIT:%.*]] = lshr i32 [[IDX]], 15
-; CHECK-NEXT: [[LOBIT_NOT:%.*]] = xor i32 [[LOBIT]], 1
-; CHECK-NEXT: [[SELECT]] = add nuw i32 [[LOBIT_NOT]], [[IDX]]
+; CHECK-NEXT: [[LOBIT:%.*]] = lshr i16 [[TMP1]], 15
+; CHECK-NEXT: [[LOBIT_NOT:%.*]] = xor i16 [[LOBIT]], 1
+; CHECK-NEXT: [[SELECT]] = add nuw i16 [[LOBIT_NOT]], [[TMP1]]
; CHECK-NEXT: br label [[FOR_BODY]]
; CHECK: exit:
-; CHECK-NEXT: [[RES:%.*]] = phi i32 [ [[TMP2]], [[IF_THEN]] ], [ 0, [[FOR_BODY]] ]
-; CHECK-NEXT: [[TMP3:%.*]] = trunc i32 [[RES]] to i16
+; CHECK-NEXT: [[TMP3:%.*]] = phi i16 [ [[LD]], [[IF_THEN]] ], [ 0, [[FOR_BODY]] ]
; CHECK-NEXT: ret i16 [[TMP3]]
;
entry:
diff --git a/llvm/test/Transforms/TypePromotion/ARM/pointers.ll b/llvm/test/Transforms/TypePromotion/ARM/pointers.ll
index 362b4ec73401c..187fc8b7267b8 100644
--- a/llvm/test/Transforms/TypePromotion/ARM/pointers.ll
+++ b/llvm/test/Transforms/TypePromotion/ARM/pointers.ll
@@ -4,20 +4,18 @@
define void @phi_pointers(ptr %a, ptr %b, i8 zeroext %M, i8 zeroext %N) {
; CHECK-LABEL: @phi_pointers(
; CHECK-NEXT: entry:
-; CHECK-NEXT: [[TMP0:%.*]] = zext i8 [[M:%.*]] to i32
-; CHECK-NEXT: [[TMP1:%.*]] = zext i8 [[N:%.*]] to i32
-; CHECK-NEXT: [[ADD:%.*]] = add nuw i32 [[TMP0]], 1
-; CHECK-NEXT: [[AND:%.*]] = and i32 [[ADD]], 1
-; CHECK-NEXT: [[CMP:%.*]] = icmp ugt i32 [[ADD]], [[TMP1]]
+; CHECK-NEXT: [[ADD:%.*]] = add nuw i8 [[M:%.*]], 1
+; CHECK-NEXT: [[AND:%.*]] = and i8 [[ADD]], 1
+; CHECK-NEXT: [[CMP:%.*]] = icmp ugt i8 [[ADD]], [[N:%.*]]
; CHECK-NEXT: [[BASE:%.*]] = select i1 [[CMP]], ptr [[A:%.*]], ptr [[B:%.*]]
; CHECK-NEXT: [[OTHER:%.*]] = select i1 [[CMP]], ptr [[B]], ptr [[B]]
; CHECK-NEXT: br label [[LOOP:%.*]]
; CHECK: loop:
; CHECK-NEXT: [[PTR:%.*]] = phi ptr [ [[BASE]], [[ENTRY:%.*]] ], [ [[GEP:%.*]], [[LOOP]] ]
-; CHECK-NEXT: [[IDX:%.*]] = phi i32 [ [[AND]], [[ENTRY]] ], [ [[INC:%.*]], [[LOOP]] ]
+; CHECK-NEXT: [[IDX:%.*]] = phi i8 [ [[AND]], [[ENTRY]] ], [ [[INC:%.*]], [[LOOP]] ]
; CHECK-NEXT: [[LOAD:%.*]] = load i16, ptr [[PTR]], align 2
-; CHECK-NEXT: [[INC]] = add nuw nsw i32 [[IDX]], 1
-; CHECK-NEXT: [[GEP]] = getelementptr inbounds i16, ptr [[PTR]], i32 [[INC]]
+; CHECK-NEXT: [[INC]] = add nuw nsw i8 [[IDX]], 1
+; CHECK-NEXT: [[GEP]] = getelementptr inbounds i16, ptr [[PTR]], i8 [[INC]]
; CHECK-NEXT: [[COND:%.*]] = icmp eq ptr [[GEP]], [[OTHER]]
; CHECK-NEXT: br i1 [[COND]], label [[EXIT:%.*]], label [[LOOP]]
; CHECK: exit:
@@ -47,11 +45,9 @@ exit:
define void @phi_pointers_null(ptr %a, ptr %b, i8 zeroext %M, i8 zeroext %N) {
; CHECK-LABEL: @phi_pointers_null(
; CHECK-NEXT: entry:
-; CHECK-NEXT: [[TMP0:%.*]] = zext i8 [[M:%.*]] to i32
-; CHECK-NEXT: [[TMP1:%.*]] = zext i8 [[N:%.*]] to i32
-; CHECK-NEXT: [[ADD:%.*]] = add nuw i32 [[TMP0]], 1
-; CHECK-NEXT: [[AND:%.*]] = and i32 [[ADD]], 1
-; CHECK-NEXT: [[CMP:%.*]] = icmp ugt i32 [[ADD]], [[TMP1]]
+; CHECK-NEXT: [[ADD:%.*]] = add nuw i8 [[M:%.*]], 1
+; CHECK-NEXT: [[AND:%.*]] = and i8 [[ADD]], 1
+; CHECK-NEXT: [[CMP:%.*]] = icmp ugt i8 [[ADD]], [[N:%.*]]
; CHECK-NEXT: [[BASE:%.*]] = select i1 [[CMP]], ptr [[A:%.*]], ptr [[B:%.*]]
; CHECK-NEXT: [[OTHER:%.*]] = select i1 [[CMP]], ptr [[B]], ptr [[B]]
; CHECK-NEXT: [[CMP_1:%.*]] = icmp eq ptr [[BASE]], [[OTHER]]
@@ -60,13 +56,13 @@ define void @phi_pointers_null(ptr %a, ptr %b, i8 zeroext %M, i8 zeroext %N) {
; CHECK-NEXT: br label [[LOOP]]
; CHECK: loop:
; CHECK-NEXT: [[PTR:%.*]] = phi ptr [ [[BASE]], [[ENTRY:%.*]] ], [ null, [[FAIL]] ], [ [[GEP:%.*]], [[IF_THEN:%.*]] ]
-; CHECK-NEXT: [[IDX:%.*]] = phi i32 [ [[AND]], [[ENTRY]] ], [ 0, [[FAIL]] ], [ [[INC:%.*]], [[IF_THEN]] ]
+; CHECK-NEXT: [[IDX:%.*]] = phi i8 [ [[AND]], [[ENTRY]] ], [ 0, [[FAIL]] ], [ [[INC:%.*]], [[IF_THEN]] ]
; CHECK-NEXT: [[UNDEF:%.*]] = icmp eq ptr [[PTR]], undef
; CHECK-NEXT: br i1 [[UNDEF]], label [[EXIT:%.*]], label [[IF_THEN]]
; CHECK: if.then:
; CHECK-NEXT: [[LOAD:%.*]] = load i16, ptr [[PTR]], align 2
-; CHECK-NEXT: [[INC]] = add nuw nsw i32 [[IDX]], 1
-; CHECK-NEXT: [[GEP]] = getelementptr inbounds i16, ptr [[PTR]], i32 [[INC]]
+; CHECK-NEXT: [[INC]] = add nuw nsw i8 [[IDX]], 1
+; CHECK-NEXT: [[GEP]] = getelementptr inbounds i16, ptr [[PTR]], i8 [[INC]]
; CHECK-NEXT: [[COND:%.*]] = icmp eq ptr [[GEP]], [[OTHER]]
; CHECK-NEXT: br i1 [[COND]], label [[EXIT]], label [[LOOP]]
; CHECK: exit:
|
Contributor
Author
|
This should match the original intent of the function, as per comment. We don't seem to bother with sign-extending here, but can change this to sext instead of bailing out, if needed. |
Contributor
Author
|
@davemgreen Is there any particular reason why we do not run isTriviallyDead here in TypePromotion, as happens in other parts in CodeGen? We convert an unused trunc to a used zext in the snippet in #133928. This would impact 22 tests primarily among AArch64 and RISCV, so I'd like this to be confirmed before adding it. |
Collaborator
|
Hi - I would guess that we usually assume that trivial instructions have already been removed in anything that wasn't a hand-written (or reduced) test case, and that the snippet for #133928 would just need to be larger if it had to include extra uses for the trivial instructions - the original problem would remain just in a different form. I don't think it would be an issue if you wanted to add it, but I doubt it would be super useful in the entire pass pipeline. I think in practice all gep indices will be canonicalized to ptrsize so this is probably OK. Can you update the |
antoniofrighetto
force-pushed
the
feature/typepromote-geps-indexes
branch
from
f3eb0ae to
5eef24d
Compare
Contributor
Author
That makes sense, although perhaps some earlier pruning in TypePromotion could alleviate a bit of work in CGP, which is scheduled to run next and recursively deletes trivially dead instructions.
Updated, thanks! |
A miscompilation issue has been addressed with improved handling. Fixes: llvm#133928. Alive2: https://alive2.llvm.org/ce/z/gcMNvS.
antoniofrighetto
force-pushed
the
feature/typepromote-geps-indexes
branch
from
dd582b6 to
13faa81
Compare
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
A miscompilation issue has been addressed with improved handling.
Fixes: #133928.
Alive2 for signext_bitcast_phi_select test: https://alive2.llvm.org/ce/z/gcMNvS.