[CodeGenPrepare] Replace deleted ext instr with the promoted value. #71058

dtcxzyw · 2023-11-02T12:41:32Z

This PR replaces the deleted ext with the promoted value in AddrMode.
Fixes #70938.

llvmbot · 2023-11-02T12:42:08Z

@llvm/pr-subscribers-backend-x86

Author: Yingwei Zheng (dtcxzyw)

Changes

This PR replaces the deleted sext with the promoted value in AddrMode.
Fixes #70938.

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

2 Files Affected:

(modified) llvm/lib/CodeGen/CodeGenPrepare.cpp (+12)
(modified) llvm/test/CodeGen/X86/codegen-prepare-addrmode-sext.ll (+22)

diff --git a/llvm/lib/CodeGen/CodeGenPrepare.cpp b/llvm/lib/CodeGen/CodeGenPrepare.cpp
index 187820717b6fd5c..9018b87835b775f 100644
--- a/llvm/lib/CodeGen/CodeGenPrepare.cpp
+++ b/llvm/lib/CodeGen/CodeGenPrepare.cpp
@@ -2634,6 +2634,15 @@ struct ExtAddrMode : public TargetLowering::AddrMode {
   void print(raw_ostream &OS) const;
   void dump() const;
 
+  void replaceWith(Value *From, Value *To) {
+    if (BaseReg == From)
+      BaseReg = To;
+    if (ScaledReg == From)
+      ScaledReg = To;
+    if (OriginalValue == From)
+      OriginalValue = To;
+  }
+
   FieldName compare(const ExtAddrMode &other) {
     // First check that the types are the same on each field, as differing types
     // is something we can't cope with later on.
@@ -4933,6 +4942,9 @@ bool AddressingModeMatcher::matchOperationAddr(User *AddrInst, unsigned Opcode,
       TPT.rollback(LastKnownGood);
       return false;
     }
+
+    // SExt has been deleted. Make sure it is not referenced by the AddrMode.
+    AddrMode.replaceWith(Ext, PromotedOperand);
     return true;
   }
   }
diff --git a/llvm/test/CodeGen/X86/codegen-prepare-addrmode-sext.ll b/llvm/test/CodeGen/X86/codegen-prepare-addrmode-sext.ll
index 6e95c91e7398932..a7f6f803d58fd4c 100644
--- a/llvm/test/CodeGen/X86/codegen-prepare-addrmode-sext.ll
+++ b/llvm/test/CodeGen/X86/codegen-prepare-addrmode-sext.ll
@@ -507,3 +507,25 @@ define i8 @oneArgPromotionBlockSExtZExt(i1 %arg1, ptr %base) {
   %res = load i8, ptr %arrayidx
   ret i8 %res
 }
+
+; Check that we replace the deleted sext with the promoted value.
+; CHECK-LABEL: define void @pr70938(
+; CHECK-SAME: ptr [[F:%.*]]) {
+; CHECK-NEXT:  entry:
+; CHECK-NEXT:    [[ADD:%.*]] = add nsw i64 0, 0
+; CHECK-NEXT:    [[SUNKADDR:%.*]] = mul i64 [[ADD]], 2
+; CHECK-NEXT:    [[SUNKADDR1:%.*]] = getelementptr i8, ptr [[F]], i64 [[SUNKADDR]]
+; CHECK-NEXT:    store i8 0, ptr [[SUNKADDR1]], align 1
+; CHECK-NEXT:    ret void
+define void @pr70938(ptr %f) {
+entry:
+  %add = add nsw i32 0, 0
+  %idxprom3 = sext i32 %add to i64
+  %arrayidx4 = getelementptr [2 x [1 x [2 x i8]]], ptr %f, i64 0, i64 %idxprom3
+  %arrayidx8 = getelementptr [2 x i8], ptr %arrayidx4, i64 0, i64 %idxprom3
+  br label %if.end
+
+if.end:                                           ; preds = %entry
+  store i8 0, ptr %arrayidx8, align 1
+  ret void
+}

llvm/test/CodeGen/X86/codegen-prepare-addrmode-sext.ll

llvm/lib/CodeGen/CodeGenPrepare.cpp

fhahn · 2023-11-03T15:03:29Z

llvm/lib/CodeGen/CodeGenPrepare.cpp

@@ -2634,6 +2634,15 @@ struct ExtAddrMode : public TargetLowering::AddrMode {
  void print(raw_ostream &OS) const;
  void dump() const;

+  void replaceWith(Value *From, Value *To) {
+    if (BaseReg == From)


Are all cases below covered by the one new test?

No. I have no idea how to generate tests that would cover all these branches :(

Do any of the duplicates provide a test case for the other cases?

These two test cases only cover if (ScaledReg == From) ScaledReg = To;.

Should we only keep the second case since existing tests do not cover other cases?

Sounds reasonable.

llvm/test/CodeGen/X86/codegen-prepare-addrmode-sext.ll

dtcxzyw · 2023-11-11T09:36:40Z

Ping.

nikic · 2023-11-14T15:24:36Z

Can you say at which point the sext is assigned to which member of the AddrMode and at which point it is later used again?

Also, does this have any relation to what the MovedAway flag is trying to handle?

arsenm · 2024-03-19T11:32:53Z

Needs rebase? Is this still relevant?

github-actions · 2025-01-08T10:52:26Z

✅ With the latest revision this PR passed the undef deprecator.

dtcxzyw · 2025-01-08T11:56:45Z

Can you say at which point the sext is assigned to which member of the AddrMode and at which point it is later used again?

For the second case, the zext is assigned to AddrMode.ScaledReg at

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

Line 4527 in bfa711a

TestAddrMode.ScaledReg = AddLHS;

.

After deleting the zext, it is used by

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

Lines 5966 to 5978 in bfa711a

    
           Value *V = AddrMode.ScaledReg; 
        
           if (V->getType() == IntPtrTy) { 
        
             // done. 
        
           } else { 
        
             assert(cast<IntegerType>(IntPtrTy)->getBitWidth() < 
        
                        cast<IntegerType>(V->getType())->getBitWidth() && 
        
                    "We can't transform if ScaledReg is too narrow"); 
        
             V = Builder.CreateTrunc(V, IntPtrTy, "sunkaddr"); 
        
           } 
        
           if (AddrMode.Scale != 1) 
        
             V = Builder.CreateMul(V, ConstantInt::get(IntPtrTy, AddrMode.Scale), 
        
                                   "sunkaddr");

Also, does this have any relation to what the MovedAway flag is trying to handle?

MovedAway only avoids using the deleted value in subsequent calls. However, the value may be used by the outer AddressingModeMatcher::matchAddr call.

llvm/test/CodeGen/X86/codegen-prepare-addrmode-sext.ll

…l` (#122101) Needed by #71058

…mode-sext.ll` (#122101) Needed by llvm/llvm-project#71058

nikic

LGTM

…value.

…value. Add comments for `replaceWith`.

nikic · 2025-01-30T08:13:01Z

/cherry-pick 3c6aa04

llvmbot · 2025-01-30T08:18:31Z

/pull-request #125040

…lvm#71058) This PR replaces the deleted ext with the promoted value in `AddrMode`. Fixes llvm#70938. (cherry picked from commit 3c6aa04)

dtcxzyw requested review from arsenm, nikic, davemgreen, bjope and efriedma-quic November 2, 2023 12:41

llvmbot added the backend:X86 label Nov 2, 2023

bjope reviewed Nov 2, 2023

View reviewed changes

llvm/test/CodeGen/X86/codegen-prepare-addrmode-sext.ll Show resolved Hide resolved

arsenm reviewed Nov 3, 2023

View reviewed changes

llvm/test/CodeGen/X86/codegen-prepare-addrmode-sext.ll Show resolved Hide resolved

fhahn reviewed Nov 3, 2023

View reviewed changes

dtcxzyw mentioned this pull request Dec 11, 2024

[clang] Crash at -O1: Assertion `materialized_use_empty() && "Uses remain when a value is destroyed!"' failed. #119429

Closed

dtcxzyw force-pushed the fix-cgp-addrmode branch from 8479b9d to e687f82 Compare January 8, 2025 10:49

dtcxzyw changed the title ~~[CodeGenPrepare] Replace deleted sext instr with the promoted value.~~ [CodeGenPrepare] Replace deleted ext instr with the promoted value. Jan 8, 2025

nikic reviewed Jan 8, 2025

View reviewed changes

llvm/test/CodeGen/X86/codegen-prepare-addrmode-sext.ll Show resolved Hide resolved

dtcxzyw mentioned this pull request Jan 8, 2025

[CodeGenPrepare] Regenerate test X86/codegen-prepare-addrmode-sext.ll #122101

Merged

dtcxzyw added a commit that referenced this pull request Jan 8, 2025

[CodeGenPrepare] Regenerate test `X86/codegen-prepare-addrmode-sext.l…

7060d2a

…l` (#122101) Needed by #71058

dtcxzyw force-pushed the fix-cgp-addrmode branch from e687f82 to 9579db1 Compare January 8, 2025 12:35

github-actions bot pushed a commit to arm/arm-toolchain that referenced this pull request Jan 10, 2025

Automerge: [CodeGenPrepare] Regenerate test `X86/codegen-prepare-addr…

49576f0

…mode-sext.ll` (#122101) Needed by llvm/llvm-project#71058

nikic mentioned this pull request Jan 23, 2025

[clang] Crash at -O1 #123884

Closed

nikic approved these changes Jan 29, 2025

View reviewed changes

dtcxzyw added 4 commits January 30, 2025 00:38

[CodeGenPrepare] Replace deleted sext instr with the promoted value.

17e2fc6

fixup! [CodeGenPrepare] Replace deleted sext instr with the promoted …

65eccd2

…value.

fixup! [CodeGenPrepare] Replace deleted sext instr with the promoted …

2ca0d6d

…value. Add comments for `replaceWith`.

[CodeGenPrepare] Regenerate tests. NFC.

7c18708

dtcxzyw added 2 commits January 30, 2025 00:38

[CodeGenPrepare] Add more tests. NFC.

2c9970d

[CodeGenPrepare] Remove uncovered code.

f4be2b6

dtcxzyw force-pushed the fix-cgp-addrmode branch from 9579db1 to f4be2b6 Compare January 29, 2025 16:44

dtcxzyw merged commit 3c6aa04 into llvm:main Jan 30, 2025
8 checks passed

dtcxzyw deleted the fix-cgp-addrmode branch January 30, 2025 00:58

nikic added this to the LLVM 20.X Release milestone Jan 30, 2025

[CodeGenPrepare] Replace deleted ext instr with the promoted value. #71058

[CodeGenPrepare] Replace deleted ext instr with the promoted value. #71058

Uh oh!

Conversation

dtcxzyw commented Nov 2, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

llvmbot commented Nov 2, 2023

Uh oh!

Uh oh!

Uh oh!

Uh oh!

fhahn Nov 3, 2023

Choose a reason for hiding this comment

Uh oh!

dtcxzyw Nov 3, 2023

Choose a reason for hiding this comment

Uh oh!

nikic Jan 7, 2025

Choose a reason for hiding this comment

Uh oh!

dtcxzyw Jan 8, 2025

Choose a reason for hiding this comment

Uh oh!

dtcxzyw Jan 29, 2025

Choose a reason for hiding this comment

Uh oh!

nikic Jan 29, 2025

Choose a reason for hiding this comment

Uh oh!

Uh oh!

dtcxzyw commented Nov 11, 2023

Uh oh!

nikic commented Nov 14, 2023

Uh oh!

arsenm commented Mar 19, 2024

Uh oh!

github-actions bot commented Jan 8, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

dtcxzyw commented Jan 8, 2025

Uh oh!

Uh oh!

nikic left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

nikic commented Jan 30, 2025

Uh oh!

llvmbot commented Jan 30, 2025

Uh oh!

Uh oh!

dtcxzyw commented Nov 2, 2023 •

edited

Loading

github-actions bot commented Jan 8, 2025 •

edited

Loading