[CGP] Fix missing sign extension for base offset in optimizeMemoryInst

[vladimirradosavljevic]

If we have integers larger than 64-bit we need to explicitly sign extend them, otherwise we will get wrong zero extended values.

[vladimirradosavljevic]

@nikic PTAL.

[llvmbot]

@llvm/pr-subscribers-llvm-transforms

Author: Vladimir Radosavljevic (vladimirradosavljevic)

Changes

If we have integers larger than 64-bit we need to explicitly sign extend them, otherwise we will get wrong zero extended values.

---

Full diff: https://github.com/llvm/llvm-project/pull/161377.diff

2 Files Affected:

• (modified) llvm/lib/CodeGen/CodeGenPrepare.cpp (+2-2)
• (added) llvm/test/Transforms/CodeGenPrepare/X86/baseoffs-sext-bug.ll (+52)

diff --git a/llvm/lib/CodeGen/CodeGenPrepare.cpp b/llvm/lib/CodeGen/CodeGenPrepare.cpp
index eb73d01b3558c..1a68577652f52 100644
--- a/llvm/lib/CodeGen/CodeGenPrepare.cpp
+++ b/llvm/lib/CodeGen/CodeGenPrepare.cpp
@@ -6100,7 +6100,7 @@ bool CodeGenPrepare::optimizeMemoryInst(Instruction *MemoryInst, Value *Addr,
 
       // Add in the Base Offset if present.
       if (AddrMode.BaseOffs) {
-        Value *V = ConstantInt::get(IntPtrTy, AddrMode.BaseOffs);
+        Value *V = ConstantInt::get(IntPtrTy, AddrMode.BaseOffs, true);
         if (ResultIndex) {
           // We need to add this separately from the scale above to help with
           // SDAG consecutive load/store merging.
@@ -6226,7 +6226,7 @@ bool CodeGenPrepare::optimizeMemoryInst(Instruction *MemoryInst, Value *Addr,
 
     // Add in the Base Offset if present.
     if (AddrMode.BaseOffs) {
-      Value *V = ConstantInt::get(IntPtrTy, AddrMode.BaseOffs);
+      Value *V = ConstantInt::get(IntPtrTy, AddrMode.BaseOffs, true);
       if (Result)
         Result = Builder.CreateAdd(Result, V, "sunkaddr");
       else
diff --git a/llvm/test/Transforms/CodeGenPrepare/X86/baseoffs-sext-bug.ll b/llvm/test/Transforms/CodeGenPrepare/X86/baseoffs-sext-bug.ll
new file mode 100644
index 0000000000000..a8ef80b389dac
--- /dev/null
+++ b/llvm/test/Transforms/CodeGenPrepare/X86/baseoffs-sext-bug.ll
@@ -0,0 +1,52 @@
+; NOTE: Assertions have been autogenerated by utils/update_test_checks.py UTC_ARGS: --version 6
+; RUN: opt -S -passes='require<profile-summary>,function(codegenprepare)' < %s | FileCheck --check-prefix=GEP %s
+; RUN: opt -S -passes='require<profile-summary>,function(codegenprepare)' -addr-sink-using-gep=false < %s | FileCheck --check-prefix=NO-GEP %s
+
+target triple = "x86_64--linux-gnu"
+target datalayout = "e-m:e-p0:128:128-p270:32:32-p271:32:32-p272:64:64-i64:64-i128:128-f80:128-n8:16:32:64-S128"
+; -p0:128:128 is added to ensure that transformation will be triggered.
+
+define i128 @test(i128 %arg) {
+; GEP-LABEL: define i128 @test(
+; GEP-SAME: i128 [[ARG:%.*]]) {
+; GEP-NEXT:  [[ENTRY:.*]]:
+; GEP-NEXT:    [[CMP:%.*]] = icmp ugt i128 [[ARG]], 10
+; GEP-NEXT:    br i1 [[CMP]], label %[[THEN:.*]], label %[[EXIT:.*]]
+; GEP:       [[THEN]]:
+; GEP-NEXT:    [[SUNKADDR:%.*]] = inttoptr i128 [[ARG]] to ptr
+; GEP-NEXT:    [[SUNKADDR1:%.*]] = getelementptr i8, ptr [[SUNKADDR]], i128 -32
+; GEP-NEXT:    [[LOAD:%.*]] = load i128, ptr [[SUNKADDR1]], align 16
+; GEP-NEXT:    br label %[[EXIT]]
+; GEP:       [[EXIT]]:
+; GEP-NEXT:    [[PHI:%.*]] = phi i128 [ [[LOAD]], %[[THEN]] ], [ 0, %[[ENTRY]] ]
+; GEP-NEXT:    ret i128 [[PHI]]
+;
+; NO-GEP-LABEL: define i128 @test(
+; NO-GEP-SAME: i128 [[ARG:%.*]]) {
+; NO-GEP-NEXT:  [[ENTRY:.*]]:
+; NO-GEP-NEXT:    [[CMP:%.*]] = icmp ugt i128 [[ARG]], 10
+; NO-GEP-NEXT:    br i1 [[CMP]], label %[[THEN:.*]], label %[[EXIT:.*]]
+; NO-GEP:       [[THEN]]:
+; NO-GEP-NEXT:    [[SUNKADDR:%.*]] = add i128 [[ARG]], -32
+; NO-GEP-NEXT:    [[SUNKADDR1:%.*]] = inttoptr i128 [[SUNKADDR]] to ptr
+; NO-GEP-NEXT:    [[LOAD:%.*]] = load i128, ptr [[SUNKADDR1]], align 16
+; NO-GEP-NEXT:    br label %[[EXIT]]
+; NO-GEP:       [[EXIT]]:
+; NO-GEP-NEXT:    [[PHI:%.*]] = phi i128 [ [[LOAD]], %[[THEN]] ], [ 0, %[[ENTRY]] ]
+; NO-GEP-NEXT:    ret i128 [[PHI]]
+;
+entry:
+  %add = add i128 %arg, -32
+  %cmp = icmp ugt i128 %arg, 10
+  br i1 %cmp, label %then, label %exit
+
+then:
+  %inttoptr = inttoptr i128 %add to ptr
+  %load = load i128, ptr %inttoptr, align 16
+  br label %exit
+
+exit:
+  %phi = phi i128 [ %load, %then ], [ 0, %entry ]
+  ret i128 %phi
+}
+

[vladimirradosavljevic]

@RKSimon @dtcxzyw

[dtcxzyw]

Nice catch!
Should we also use sign extend here?

llvm-project/llvm/lib/CodeGen/CodeGenPrepare.cpp

Line 3197 in 4c2b1d4

return ConstantInt::get(IntPtrTy, BaseOffs);

[nikic]

We need to implement this TODO to catch these issues:

llvm-project/llvm/lib/IR/Constants.cpp

Lines 954 to 957 in 4c2b1d4

	// TODO: Avoid implicit trunc?
	// See https://github.com/llvm/llvm-project/issues/112510.
	return get(Ty->getContext(),
	APInt(Ty->getBitWidth(), V, isSigned, /*implicitTrunc=*/true));

[vladimirradosavljevic]

Nice catch! Should we also use sign extend here?

llvm-project/llvm/lib/CodeGen/CodeGenPrepare.cpp

Line 3197 in 4c2b1d4

return ConstantInt::get(IntPtrTy, BaseOffs);

Yes, we also have to use sign extend there. Nice catch!
Thanks for the review! Could you please merge this PR on my behalf?