This is an archive of the discontinued LLVM Phabricator instance.

Differential D157513

[X86] Fix aliasing check between a TargetFrameIndex and a FrameIndex
ClosedPublic

Authored by n-omer on Aug 9 2023, 7:50 AM.

Details

Reviewers
RKSimon
pengfei
Commits
rGd06608060c8f: [X86] Fix aliasing check between TargetFrameIndex and FrameIndex
Summary

Fixes https://github.com/llvm/llvm-project/issues/63645.

While processing the DAG below, DAGCombiner will move t24 up and chain it to t13 (visitSTORE(), findBetterNeighborChains(), GatherAllAliases()) because thats the first node that aliases with it (according to GatherAllAliases()). This also means that the store is moved past t15.

This is wrong, by looking at t24 and it's operands you can see that it does alias with t15 (t22 is 0). The reason this happens is that the base pointers of the address arguments of t24 and t15 are FrameIndex:i64<0> and TargetFrameIndex:i64<0> respectively, they have different pointer values but are really pointing to the same address in the target function.

This later results in all stores initializing that stack slot being deleted and incorrect code is generated.

Initial selection DAG: %bb.0 'main:entry'
SelectionDAG has 41 nodes:
  t3: i64 = Constant<0>
  t8: i64 = add nuw FrameIndex:i64<0>, Constant:i64<4>
                  t0: ch,glue = EntryToken
                t5: ch = store<(store (s32) into @c, !tbaa !6)> t0, Constant:i32<0>, GlobalAddress:i64<ptr @c> 0, undef:i64
              t10: ch = lifetime.start<0 to 8> t5, TargetFrameIndex:i64<0>
            t12: ch = store<(store (s32) into %ir.a.i, !tbaa !6)> t10, Constant:i32<50009>, FrameIndex:i64<0>, undef:i64
          t13: ch = store<(store (s32) into %ir.arrayidx.i.1, !tbaa !6)> t12, Constant:i32<50009>, t8, undef:i64
        t14: ch = lifetime.end<0 to 8> t13, TargetFrameIndex:i64<0>
      t15: ch = lifetime.start<0 to 8> t14, TargetFrameIndex:i64<0>
    t18: ch = store<(store (s64) into @p, !tbaa !13)> t15, Constant:i64<64>, GlobalAddress:i64<ptr @p> 0, undef:i64
  t19: i32,ch = load<(dereferenceable load (s32) from @c, !tbaa !6)> t18, GlobalAddress:i64<ptr @c> 0, undef:i64
          t20: i64 = zero_extend t19
        t22: i64 = shl t20, Constant:i64<2>
      t23: i64 = add FrameIndex:i64<0>, t22
    t24: ch = store<(store (s32) into %ir.arrayidx.1.i, !tbaa !6)> t19:1, Constant:i32<50009>, t23, undef:i64
          t26: i32 = add t19, Constant:i32<1>
        t27: i64 = zero_extend t26
      t28: i64 = shl t27, Constant:i64<2>
    t29: i64 = add FrameIndex:i64<0>, t28
  t30: ch = store<(store (s32) into %ir.arrayidx.1.i.1, !tbaa !6)> t24, Constant:i32<50009>, t29, undef:i64
          t31: i32,ch = load<(dereferenceable load (s32) from %ir.arrayidx15.i, !tbaa !6)> t30, t8, undef:i64
        t34: i1 = setcc t31, Constant:i32<42829>, setlt:ch
      t36: i1 = xor t34, Constant:i1<-1>
    t38: ch = brcond t30, t36, BasicBlock:ch<t.exit 0xe93b5a8>
  t40: ch = br t38, BasicBlock:ch<if.then.1.i 0xe93b4a8>

Diff Detail

Event Timeline

n-omer created this revision.Aug 9 2023, 7:50 AM
Herald added a project: Restricted Project. · View Herald TranscriptAug 9 2023, 7:50 AM
Herald added subscribers: jeroen.dobbelaere, steven.zhang, arphaman, hiraditya. · View Herald Transcript
n-omer requested review of this revision.Aug 9 2023, 7:50 AM
Herald added a project: Restricted Project. · View Herald TranscriptAug 9 2023, 7:50 AM
Herald added a subscriber: llvm-commits. · View Herald Transcript
n-omer edited the summary of this revision. (Show Details)Aug 9 2023, 7:51 AM
n-omer added a comment.Aug 9 2023, 9:19 AM

Before and after diff of the test:

--- a/llvm/test/CodeGen/X86/pr63645.ll
+++ b/llvm/test/CodeGen/X86/pr63645.ll
@@ -17,6 +17,8 @@ define i32 @main() #1 {
 ; CHECK:       # %bb.0: # %entry
 ; CHECK-NEXT:    movl $0, c(%rip)
 ; CHECK-NEXT:    movq $64, p(%rip)
+; CHECK-NEXT:    movabsq $214787019555673, %rax # imm = 0xC3590000C359
+; CHECK-NEXT:    movq %rax, -{{[0-9]+}}(%rsp)
 ; CHECK-NEXT:    cmpl $42828, -{{[0-9]+}}(%rsp) # imm = 0xA74C
 ; CHECK-NEXT:    jg .LBB0_2
 ; CHECK-NEXT:  # %bb.1: # %if.then.1.i
n-omer edited the summary of this revision. (Show Details)Aug 9 2023, 9:28 AM
Harbormaster completed remote builds in B251389: Diff 548619.Aug 9 2023, 9:40 AM
pengfei added a comment.Aug 10 2023, 7:36 AM
In D157513#4573485, @n-omer wrote:

Before and after diff of the test:

--- a/llvm/test/CodeGen/X86/pr63645.ll
+++ b/llvm/test/CodeGen/X86/pr63645.ll
@@ -17,6 +17,8 @@ define i32 @main() #1 {
 ; CHECK:       # %bb.0: # %entry
 ; CHECK-NEXT:    movl $0, c(%rip)
 ; CHECK-NEXT:    movq $64, p(%rip)
+; CHECK-NEXT:    movabsq $214787019555673, %rax # imm = 0xC3590000C359
+; CHECK-NEXT:    movq %rax, -{{[0-9]+}}(%rsp)
 ; CHECK-NEXT:    cmpl $42828, -{{[0-9]+}}(%rsp) # imm = 0xA74C
 ; CHECK-NEXT:    jg .LBB0_2
 ; CHECK-NEXT:  # %bb.1: # %if.then.1.i

You can pre-commit the test and only show the diff in this patch.

n-omer mentioned this in rGd43634cd743d: [X86] Pre-commit test for D157513.Aug 10 2023, 7:55 AM
n-omer updated this revision to Diff 549047.Aug 10 2023, 8:00 AM

Rebase.

Harbormaster completed remote builds in B251697: Diff 549047.Aug 10 2023, 1:03 PM
n-omer added a comment.Aug 16 2023, 1:26 PM

Hi @pengfei, are you happy with the patch now?

pengfei accepted this revision.Aug 16 2023, 7:48 PM

LGTM, thanks!

This revision is now accepted and ready to land.Aug 16 2023, 7:48 PM
This revision was landed with ongoing or failed builds.Aug 17 2023, 2:48 AM
Closed by commit rGd06608060c8f: [X86] Fix aliasing check between TargetFrameIndex and FrameIndex (authored by n-omer). · Explain Why
This revision was automatically updated to reflect the committed changes.
n-omer added a commit: rGd06608060c8f: [X86] Fix aliasing check between TargetFrameIndex and FrameIndex.

Revision Contents

PathSize
llvm/
lib/
CodeGen/
SelectionDAG/
2 lines
test/
CodeGen/
X86/
2 lines

Diff 549047

llvm/lib/CodeGen/SelectionDAG/SelectionDAGAddressAnalysis.cpp

Show First 20 Lines • Show All 124 Lines • ▼ Show 20 Linesbool BaseIndexOffset::computeAliasing(const SDNode *Op0,
// able to calculate their relative offset if at least one arises // able to calculate their relative offset if at least one arises
// from an alloca. However, these allocas cannot overlap and we // from an alloca. However, these allocas cannot overlap and we
// can infer there is no alias. // can infer there is no alias.
if (auto *A = dyn_cast<FrameIndexSDNode>(BasePtr0.getBase())) if (auto *A = dyn_cast<FrameIndexSDNode>(BasePtr0.getBase()))
if (auto *B = dyn_cast<FrameIndexSDNode>(BasePtr1.getBase())) { if (auto *B = dyn_cast<FrameIndexSDNode>(BasePtr1.getBase())) {
MachineFrameInfo &MFI = DAG.getMachineFunction().getFrameInfo(); MachineFrameInfo &MFI = DAG.getMachineFunction().getFrameInfo();
// If the base are the same frame index but the we couldn't find a // If the base are the same frame index but the we couldn't find a
// constant offset, (indices are different) be conservative. // constant offset, (indices are different) be conservative.
if (A != B && (!MFI.isFixedObjectIndex(A->getIndex()) || if (A->getIndex() != B->getIndex() && (!MFI.isFixedObjectIndex(A->getIndex()) ||
!MFI.isFixedObjectIndex(B->getIndex()))) { !MFI.isFixedObjectIndex(B->getIndex()))) {
IsAlias = false; IsAlias = false;
return true; return true;
} }
} }
bool IsFI0 = isa<FrameIndexSDNode>(BasePtr0.getBase()); bool IsFI0 = isa<FrameIndexSDNode>(BasePtr0.getBase());
bool IsFI1 = isa<FrameIndexSDNode>(BasePtr1.getBase()); bool IsFI1 = isa<FrameIndexSDNode>(BasePtr1.getBase());
▲ Show 20 Lines • Show All 183 Lines • Show Last 20 Lines

llvm/test/CodeGen/X86/pr63645.ll

Show All 11 Lines
declare void @llvm.lifetime.end.p0(i64 immarg, ptr nocapture) declare void @llvm.lifetime.end.p0(i64 immarg, ptr nocapture)
; Function Attrs: nofree nosync nounwind memory(readwrite, inaccessiblemem: none) uwtable ; Function Attrs: nofree nosync nounwind memory(readwrite, inaccessiblemem: none) uwtable
define i32 @main() #1 { define i32 @main() #1 {
; CHECK-LABEL: main: ; CHECK-LABEL: main:
; CHECK: # %bb.0: # %entry ; CHECK: # %bb.0: # %entry
; CHECK-NEXT: movl $0, c(%rip) ; CHECK-NEXT: movl $0, c(%rip)
; CHECK-NEXT: movq $64, p(%rip) ; CHECK-NEXT: movq $64, p(%rip)
; CHECK-NEXT: movabsq $214787019555673, %rax # imm = 0xC3590000C359
; CHECK-NEXT: movq %rax, -{{[0-9]+}}(%rsp)
; CHECK-NEXT: cmpl $42828, -{{[0-9]+}}(%rsp) # imm = 0xA74C ; CHECK-NEXT: cmpl $42828, -{{[0-9]+}}(%rsp) # imm = 0xA74C
; CHECK-NEXT: jg .LBB0_2 ; CHECK-NEXT: jg .LBB0_2
; CHECK-NEXT: # %bb.1: # %if.then.1.i ; CHECK-NEXT: # %bb.1: # %if.then.1.i
; CHECK-NEXT: movq l(%rip), %rax ; CHECK-NEXT: movq l(%rip), %rax
; CHECK-NEXT: movb $0, (%rax) ; CHECK-NEXT: movb $0, (%rax)
; CHECK-NEXT: .LBB0_2: # %t.exit ; CHECK-NEXT: .LBB0_2: # %t.exit
; CHECK-NEXT: xorl %eax, %eax ; CHECK-NEXT: xorl %eax, %eax
; CHECK-NEXT: retq ; CHECK-NEXT: retq
Show All 33 Lines