This is an archive of the discontinued LLVM Phabricator instance.

Differential D55642

[X86] Fix assert fails in pass X86AvoidSFBPass
ClosedPublic

Authored by pengfei on Dec 12 2018, 7:47 PM.

Details

Reviewers
craig.topper
smaslov
LuoYuanke
lsaba
manojgupta
nikic
Commits
rG291470347a2e: [X86] Fix assert fails in pass X86AvoidSFBPass
rL349660: [X86] Fix assert fails in pass X86AvoidSFBPass
Summary

Fixes https://bugs.llvm.org/show_bug.cgi?id=38743

The function removeRedundantBlockingStores is supposed to remove any blocking stores contained in each other in lockingStoresDispSizeMap.
But it currently looks only at the previous one, which will miss some cases that result in assert.

This patch refine the function to check all previous layouts until find the uncontained one. So all redundant stores will be removed.

Diff Detail

Event Timeline

pengfei created this revision.Dec 12 2018, 7:47 PM
Herald added a subscriber: llvm-commits. · View Herald TranscriptDec 12 2018, 7:47 PM
pengfei edited the summary of this revision. (Show Details)Dec 12 2018, 7:50 PM
nikic added a subscriber: nikic.Dec 13 2018, 4:15 AM
nikic added inline comments.
lib/Target/X86/X86AvoidStoreForwardingBlocks.cpp
646

This can be a range-based for loop now.

test/CodeGen/X86/pr38743.ll
1

Could you please also include the generated assembly? This can be done by running utils/update_llc_test_checks.py test/CodeGen/X86/pr38743.ll. Possibly with a --llc-bin build/bin/llc argument, or similar.

pengfei updated this revision to Diff 178041.Dec 13 2018, 5:50 AM

Thanks for nikic's comments

pengfei marked 2 inline comments as done.Dec 13 2018, 5:52 AM
lsaba added a comment.Dec 13 2018, 9:59 AM

no comments from my side (LGTM)

nikic added a comment.Dec 13 2018, 11:05 AM

I think your current test case has been invalidated by D55365. In the current form, it does not assert anymore, even without this fix. Would it be possible to tweak the test case to reproduce the assertion failure, possibly by writing out some memcpys in terms of load/stores in a way that resembled the previous lowering?

pengfei updated this revision to Diff 178192.Dec 14 2018, 12:23 AM
pengfei added a reviewer: nikic.

Thanks Nikita! Otherwise I will provide an invalid test.

nikic accepted this revision.Dec 14 2018, 11:36 AM

LGTM, with a small nit.

In general, I'm not sure whether the overall behavior here makes sense. Especially in the case where the blocking stores don't come from different BBs, it would make more sense to me to preserve the last (in program order) of the overlapping stores, rather than the smallest, as that's the store that will be forwarded. If the stores come from multiple predecessors (like here) it's less clear which choice is right.

But in any case, that would require larger changes and shouldn't block this assertion fix.

lib/Target/X86/X86AvoidStoreForwardingBlocks.cpp
649–654

I don't think the const on a non-pointer here is useful.

This revision is now accepted and ready to land.Dec 14 2018, 11:36 AM
pengfei updated this revision to Diff 178440.Dec 17 2018, 4:13 AM
pengfei marked an inline comment as done.

Remove useless const.

pengfei added a comment.Dec 17 2018, 4:40 AM

@nikic Thank you very much for your auditing and suggestions.
The test here was just trimmed from the bug repro and used to verify the fix to the assertion. However it exposed that it may be better if it just optimized inside the BB in some cases. We will reserach it later.

@craig.topper Can you help me to submit this patch? Thanks a lot.

lsaba added a comment.Dec 18 2018, 7:09 AM
In D55642#1331489, @nikic wrote:

LGTM, with a small nit.

In general, I'm not sure whether the overall behavior here makes sense. Especially in the case where the blocking stores don't come from different BBs, it would make more sense to me to preserve the last (in program order) of the overlapping stores, rather than the smallest, as that's the store that will be forwarded. If the stores come from multiple predecessors (like here) it's less clear which choice is right.

But in any case, that would require larger changes and shouldn't block this assertion fix.

The idea behind using the smallest blocking store was to not introduce a new SFB between the smallest store and the newly created loads which are still bigger than it (since they are the size of the latest blocking store), I agree that improvements can be made to this patch especially for identifying and handling the behavior of blocking stores that come from predecessing blocks.

Closed by commit rL349660: [X86] Fix assert fails in pass X86AvoidSFBPass (authored by ctopper). · Explain WhyDec 19 2018, 10:49 AM
This revision was automatically updated to reflect the committed changes.

Revision Contents

PathSize
lib/
Target/
X86/
22 lines
test/
CodeGen/
X86/
57 lines

Diff 177997

lib/Target/X86/X86AvoidStoreForwardingBlocks.cpp

Show First 20 Lines • Show All 635 Lines • ▼ Show 20 Lines
} }
// Remove blocking stores contained in each other. // Remove blocking stores contained in each other.
static void static void
removeRedundantBlockingStores(DisplacementSizeMap &BlockingStoresDispSizeMap) { removeRedundantBlockingStores(DisplacementSizeMap &BlockingStoresDispSizeMap) {
if (BlockingStoresDispSizeMap.size() <= 1) if (BlockingStoresDispSizeMap.size() <= 1)
return; return;
int64_t PrevDisp = BlockingStoresDispSizeMap.begin()->first; SmallVector<std::pair<const int64_t, unsigned>, 0> DispSizeStack;
unsigned PrevSize = BlockingStoresDispSizeMap.begin()->second; for (auto DispSizePair = BlockingStoresDispSizeMap.begin();
SmallVector<int64_t, 2> ForRemoval;
for (auto DispSizePair = std::next(BlockingStoresDispSizeMap.begin());
DispSizePair != BlockingStoresDispSizeMap.end(); ++DispSizePair) { DispSizePair != BlockingStoresDispSizeMap.end(); ++DispSizePair) {
nikicUnsubmitted
Done
ReplyInline Actions

This can be a range-based for loop now.

nikic: This can be a range-based for loop now.
int64_t CurrDisp = DispSizePair->first; int64_t CurrDisp = DispSizePair->first;
unsigned CurrSize = DispSizePair->second; unsigned CurrSize = DispSizePair->second;
if (CurrDisp + CurrSize <= PrevDisp + PrevSize) { while (DispSizeStack.size()) {
ForRemoval.push_back(PrevDisp); int64_t PrevDisp = DispSizeStack.back().first;
unsigned PrevSize = DispSizeStack.back().second;
if (CurrDisp + CurrSize > PrevDisp + PrevSize)
break;
DispSizeStack.pop_back();
nikicUnsubmitted
Done
ReplyInline Actions

I don't think the const on a non-pointer here is useful.

nikic: I don't think the `const` on a non-pointer here is useful.
} }
PrevDisp = CurrDisp; DispSizeStack.push_back(*DispSizePair);
PrevSize = CurrSize;
} }
for (auto Disp : ForRemoval) BlockingStoresDispSizeMap.clear();
BlockingStoresDispSizeMap.erase(Disp); for (auto Disp : DispSizeStack)
BlockingStoresDispSizeMap.insert(Disp);
} }
bool X86AvoidSFBPass::runOnMachineFunction(MachineFunction &MF) { bool X86AvoidSFBPass::runOnMachineFunction(MachineFunction &MF) {
bool Changed = false; bool Changed = false;
if (DisableX86AvoidStoreForwardBlocks || skipFunction(MF.getFunction()) || if (DisableX86AvoidStoreForwardBlocks || skipFunction(MF.getFunction()) ||
!MF.getSubtarget<X86Subtarget>().is64Bit()) !MF.getSubtarget<X86Subtarget>().is64Bit())
return false; return false;
▲ Show 20 Lines • Show All 62 Lines • Show Last 20 Lines

test/CodeGen/X86/pr38743.ll

  • This file was added.
; RUN: llc < %s
nikicUnsubmitted
Done
ReplyInline Actions

Could you please also include the generated assembly? This can be done by running utils/update_llc_test_checks.py test/CodeGen/X86/pr38743.ll. Possibly with a --llc-bin build/bin/llc argument, or similar.

nikic: Could you please also include the generated assembly? This can be done by running…
%0 = type { %1 }
%1 = type { %2 }
%2 = type { %3 }
%3 = type { %4 }
%4 = type { %5 }
%5 = type { i64, i64, i8* }
%6 = type { %7, [23 x i8] }
%7 = type { i8 }
@.str.16 = external dso_local unnamed_addr constant [16 x i8], align 1
@.str.17 = external dso_local unnamed_addr constant [12 x i8], align 1
@.str.18 = external dso_local unnamed_addr constant [15 x i8], align 1
declare void @llvm.memcpy.p0i8.p0i8.i64(i8* nocapture writeonly, i8* nocapture readonly, i64, i1) #0
define void @pr38743() #1 align 2 {
bb:
%tmp = alloca %0, align 16
%tmp1 = bitcast %0* %tmp to i8*
switch i32 undef, label %bb11 [
i32 1, label %bb2
i32 4, label %bb5
i32 2, label %bb5
i32 3, label %bb8
]
bb2: ; preds = %bb
%tmp3 = bitcast %0* %tmp to %6*
%tmp4 = getelementptr inbounds %6, %6* %tmp3, i64 0, i32 1, i64 0
call void @llvm.memcpy.p0i8.p0i8.i64(i8* nonnull align 1 %tmp4, i8* align 1 getelementptr inbounds ([16 x i8], [16 x i8]* @.str.16, i64 0, i64 0), i64 15, i1 false)
br label %bb12
bb5: ; preds = %bb, %bb
%tmp6 = bitcast %0* %tmp to %6*
%tmp7 = getelementptr inbounds %6, %6* %tmp6, i64 0, i32 1, i64 0
call void @llvm.memcpy.p0i8.p0i8.i64(i8* nonnull align 1 %tmp7, i8* align 1 getelementptr inbounds ([12 x i8], [12 x i8]* @.str.17, i64 0, i64 0), i64 11, i1 false)
br label %bb12
bb8: ; preds = %bb
%tmp9 = bitcast %0* %tmp to %6*
%tmp10 = getelementptr inbounds %6, %6* %tmp9, i64 0, i32 1, i64 0
call void @llvm.memcpy.p0i8.p0i8.i64(i8* nonnull align 1 %tmp10, i8* align 1 getelementptr inbounds ([15 x i8], [15 x i8]* @.str.18, i64 0, i64 0), i64 14, i1 false)
br label %bb12
bb11: ; preds = %bb
unreachable
bb12: ; preds = %bb8, %bb5, %bb2
call void @llvm.memcpy.p0i8.p0i8.i64(i8* nonnull align 8 undef, i8* nonnull align 16 %tmp1, i64 24, i1 false) #2
ret void
}
attributes #0 = { argmemonly nounwind }
attributes #1 = { "target-features"="+sse,+sse2,+sse3,+sse4.2" }
attributes #2 = { nounwind }