Skip to content

Commit fec2945

Browse files
committed
Revert "[GVN] Clobber partially aliased loads."
This reverts commit 6c57044. It causes assertion errors due to widening atomic loads, and potentially causes miscompile elsewhere too. Repro, also posted to D95543: ``` $ cat repro.ll ; ModuleID = 'repro.ll' source_filename = "repro.ll" target datalayout = "e-m:e-p270:32:32-p271:32:32-p272:64:64-i64:64-f80:128-n8:16:32:64-S128" target triple = "x86_64-unknown-linux-gnu" %struct.widget = type { i32 } %struct.baz = type { i32, %struct.snork } %struct.snork = type { %struct.spam } %struct.spam = type { i32, i32 } @global = external local_unnamed_addr global %struct.widget, align 4 @global.1 = external local_unnamed_addr global i8, align 1 @global.2 = external local_unnamed_addr global i32, align 4 define void @zot(%struct.baz* %arg) local_unnamed_addr align 2 { bb: %tmp = getelementptr inbounds %struct.baz, %struct.baz* %arg, i64 0, i32 1 %tmp1 = bitcast %struct.snork* %tmp to i64* %tmp2 = load i64, i64* %tmp1, align 4 %tmp3 = getelementptr inbounds %struct.baz, %struct.baz* %arg, i64 0, i32 1, i32 0, i32 1 %tmp4 = icmp ugt i64 %tmp2, 4294967295 br label %bb5 bb5: ; preds = %bb14, %bb %tmp6 = load i32, i32* %tmp3, align 4 %tmp7 = icmp ne i32 %tmp6, 0 %tmp8 = select i1 %tmp7, i1 %tmp4, i1 false %tmp9 = zext i1 %tmp8 to i8 store i8 %tmp9, i8* @global.1, align 1 %tmp10 = load i32, i32* @global.2, align 4 switch i32 %tmp10, label %bb11 [ i32 1, label %bb12 i32 2, label %bb12 ] bb11: ; preds = %bb5 br label %bb14 bb12: ; preds = %bb5, %bb5 %tmp13 = load atomic i32, i32* getelementptr inbounds (%struct.widget, %struct.widget* @global, i64 0, i32 0) acquire, align 4 br label %bb14 bb14: ; preds = %bb12, %bb11 br label %bb5 } $ opt -O2 repro.ll -disable-output opt: /home/rupprecht/src/llvm-project/llvm/lib/Transforms/Utils/VNCoercion.cpp:496: llvm::Value *llvm::VNCoercion::getLoadValueForLoad(llvm::LoadInst *, unsigned int, llvm::Type *, llvm::Instruction *, const llvm::DataLayout &): Assertion `SrcVal->isSimple() && "Cannot widen volatile/atomic load!"' failed. PLEASE submit a bug report to https://bugs.llvm.org/ and include the crash backtrace. Stack dump: 0. Program arguments: /home/rupprecht/dev/opt -O2 repro.ll -disable-output ... ```
1 parent d63860a commit fec2945

File tree

4 files changed

+67
-212
lines changed

4 files changed

+67
-212
lines changed

llvm/include/llvm/Analysis/MemoryDependenceAnalysis.h

Lines changed: 0 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -363,7 +363,6 @@ class MemoryDependenceResults {
363363
PredIteratorCache PredCache;
364364

365365
unsigned DefaultBlockScanLimit;
366-
Optional<int32_t> ClobberOffset;
367366

368367
public:
369368
MemoryDependenceResults(AAResults &AA, AssumptionCache &AC,
@@ -469,8 +468,6 @@ class MemoryDependenceResults {
469468
/// Release memory in caches.
470469
void releaseMemory();
471470

472-
Optional<int32_t> getClobberOffset() const { return ClobberOffset; }
473-
474471
private:
475472
MemDepResult getCallDependencyFrom(CallBase *Call, bool isReadOnlyCall,
476473
BasicBlock::iterator ScanIt,

llvm/lib/Analysis/MemoryDependenceAnalysis.cpp

Lines changed: 7 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -512,12 +512,16 @@ MemDepResult MemoryDependenceResults::getSimplePointerDependencyFrom(
512512
if (R == AliasResult::MustAlias)
513513
return MemDepResult::getDef(Inst);
514514

515+
#if 0 // FIXME: Temporarily disabled. GVN is cleverly rewriting loads
516+
// in terms of clobbering loads, but since it does this by looking
517+
// at the clobbering load directly, it doesn't know about any
518+
// phi translation that may have happened along the way.
519+
515520
// If we have a partial alias, then return this as a clobber for the
516521
// client to handle.
517-
if (R == AliasResult::PartialAlias && R.hasOffset()) {
518-
ClobberOffset = R.getOffset();
522+
if (R == AliasResult::PartialAlias)
519523
return MemDepResult::getClobber(Inst);
520-
}
524+
#endif
521525

522526
// Random may-alias loads don't depend on each other without a
523527
// dependence.
@@ -636,7 +640,6 @@ MemDepResult MemoryDependenceResults::getSimplePointerDependencyFrom(
636640
}
637641

638642
MemDepResult MemoryDependenceResults::getDependency(Instruction *QueryInst) {
639-
ClobberOffset = None;
640643
Instruction *ScanPos = QueryInst;
641644

642645
// Check for a cached result

llvm/lib/Transforms/Scalar/GVN.cpp

Lines changed: 3 additions & 16 deletions
Original file line numberDiff line numberDiff line change
@@ -999,22 +999,9 @@ bool GVN::AnalyzeLoadAvailability(LoadInst *Load, MemDepResult DepInfo,
999999
// Can't forward from non-atomic to atomic without violating memory model.
10001000
if (DepLoad != Load && Address &&
10011001
Load->isAtomic() <= DepLoad->isAtomic()) {
1002-
Type *LoadType = Load->getType();
1003-
int Offset = -1;
1004-
1005-
// If Memory Dependence Analysis reported clobber check, it was nested
1006-
// and can be extracted from the MD result
1007-
if (DepInfo.isClobber() &&
1008-
canCoerceMustAliasedValueToLoad(DepLoad, LoadType, DL)) {
1009-
const auto ClobberOff = MD->getClobberOffset();
1010-
// GVN has no deal with a negative offset.
1011-
Offset = (ClobberOff == None || ClobberOff.getValue() < 0)
1012-
? -1
1013-
: ClobberOff.getValue();
1014-
}
1015-
if (Offset == -1)
1016-
Offset =
1017-
analyzeLoadFromClobberingLoad(LoadType, Address, DepLoad, DL);
1002+
int Offset = analyzeLoadFromClobberingLoad(Load->getType(), Address,
1003+
DepLoad, DL);
1004+
10181005
if (Offset != -1) {
10191006
Res = AvailableValue::getLoad(DepLoad, Offset);
10201007
return true;

0 commit comments

Comments
 (0)