[Loads] Check context instruction for context-sensitive derefability #109277
Conversation
llvmbot
added
llvm:analysis
|
@llvm/pr-subscribers-clang @llvm/pr-subscribers-llvm-analysis Author: Nikita Popov (nikic) ChangesIf a dereferenceability fact is provided through Fix this by checking whether the instruction with Fixes #108854. Full diff: https://github.com/llvm/llvm-project/pull/109277.diff 4 Files Affected:
diff --git a/llvm/lib/Analysis/Loads.cpp b/llvm/lib/Analysis/Loads.cpp
index 957ac883490c45..11f3807ffacf6e 100644
--- a/llvm/lib/Analysis/Loads.cpp
+++ b/llvm/lib/Analysis/Loads.cpp
@@ -104,6 +104,17 @@ static bool isDereferenceableAndAlignedPointer(
if (CheckForNonNull &&
!isKnownNonZero(V, SimplifyQuery(DL, DT, AC, CtxI)))
return false;
+ // When using something like !dereferenceable on a load, the
+ // dereferenceability may only be valid on a specific control-flow path.
+ // If the instruction doesn't dominate the context instruction, we're
+ // asking about dereferenceability under the assumption that the
+ // instruction has been speculated to the point of the context instruction,
+ // in which case we don't know if the dereferenceability info still holds.
+ // We don't bother handling allocas here, as they aren't speculatable
+ // anyway.
+ auto *I = dyn_cast<Instruction>(V);
+ if (I && !isa<AllocaInst>(I))
+ return CtxI && isValidAssumeForContext(I, CtxI, DT);
return true;
};
if (IsKnownDeref()) {
diff --git a/llvm/lib/Analysis/MemDerefPrinter.cpp b/llvm/lib/Analysis/MemDerefPrinter.cpp
index e858d941435441..68cb8859488f70 100644
--- a/llvm/lib/Analysis/MemDerefPrinter.cpp
+++ b/llvm/lib/Analysis/MemDerefPrinter.cpp
@@ -30,10 +30,10 @@ PreservedAnalyses MemDerefPrinterPass::run(Function &F,
for (auto &I : instructions(F)) {
if (LoadInst *LI = dyn_cast<LoadInst>(&I)) {
Value *PO = LI->getPointerOperand();
- if (isDereferenceablePointer(PO, LI->getType(), DL))
+ if (isDereferenceablePointer(PO, LI->getType(), DL, LI))
Deref.push_back(PO);
if (isDereferenceableAndAlignedPointer(PO, LI->getType(), LI->getAlign(),
- DL))
+ DL, LI))
DerefAndAligned.insert(PO);
}
}
diff --git a/llvm/lib/CodeGen/MachineOperand.cpp b/llvm/lib/CodeGen/MachineOperand.cpp
index 6ee47624f31c54..89d32c3f005e00 100644
--- a/llvm/lib/CodeGen/MachineOperand.cpp
+++ b/llvm/lib/CodeGen/MachineOperand.cpp
@@ -1047,7 +1047,8 @@ bool MachinePointerInfo::isDereferenceable(unsigned Size, LLVMContext &C,
return false;
return isDereferenceableAndAlignedPointer(
- BasePtr, Align(1), APInt(DL.getPointerSizeInBits(), Offset + Size), DL);
+ BasePtr, Align(1), APInt(DL.getPointerSizeInBits(), Offset + Size), DL,
+ dyn_cast<Instruction>(BasePtr));
}
/// getConstantPool - Return a MachinePointerInfo record that refers to the
diff --git a/llvm/test/Transforms/SimplifyCFG/speculate-derefable-load.ll b/llvm/test/Transforms/SimplifyCFG/speculate-derefable-load.ll
index 8c7afa4598bd4b..0138433312ed84 100644
--- a/llvm/test/Transforms/SimplifyCFG/speculate-derefable-load.ll
+++ b/llvm/test/Transforms/SimplifyCFG/speculate-derefable-load.ll
@@ -77,14 +77,17 @@ exit:
ret i64 %res
}
-; FIXME: This is a miscompile.
define i64 @deref_no_hoist(i1 %c, ptr align 8 dereferenceable(8) %p1) {
; CHECK-LABEL: define i64 @deref_no_hoist(
; CHECK-SAME: i1 [[C:%.*]], ptr align 8 dereferenceable(8) [[P1:%.*]]) {
-; CHECK-NEXT: [[ENTRY:.*:]]
-; CHECK-NEXT: [[P2:%.*]] = load ptr, ptr [[P1]], align 8, !align [[META0:![0-9]+]]
+; CHECK-NEXT: [[ENTRY:.*]]:
+; CHECK-NEXT: br i1 [[C]], label %[[IF:.*]], label %[[EXIT:.*]]
+; CHECK: [[IF]]:
+; CHECK-NEXT: [[P2:%.*]] = load ptr, ptr [[P1]], align 8, !dereferenceable [[META0:![0-9]+]], !align [[META0]]
; CHECK-NEXT: [[V:%.*]] = load i64, ptr [[P2]], align 8
-; CHECK-NEXT: [[RES:%.*]] = select i1 [[C]], i64 [[V]], i64 0
+; CHECK-NEXT: br label %[[EXIT]]
+; CHECK: [[EXIT]]:
+; CHECK-NEXT: [[RES:%.*]] = phi i64 [ [[V]], %[[IF]] ], [ 0, %[[ENTRY]] ]
; CHECK-NEXT: ret i64 [[RES]]
;
entry:
|
|
@llvm/pr-subscribers-llvm-transforms Author: Nikita Popov (nikic) ChangesIf a dereferenceability fact is provided through Fix this by checking whether the instruction with Fixes #108854. Full diff: https://github.com/llvm/llvm-project/pull/109277.diff 4 Files Affected:
diff --git a/llvm/lib/Analysis/Loads.cpp b/llvm/lib/Analysis/Loads.cpp
index 957ac883490c45..11f3807ffacf6e 100644
--- a/llvm/lib/Analysis/Loads.cpp
+++ b/llvm/lib/Analysis/Loads.cpp
@@ -104,6 +104,17 @@ static bool isDereferenceableAndAlignedPointer(
if (CheckForNonNull &&
!isKnownNonZero(V, SimplifyQuery(DL, DT, AC, CtxI)))
return false;
+ // When using something like !dereferenceable on a load, the
+ // dereferenceability may only be valid on a specific control-flow path.
+ // If the instruction doesn't dominate the context instruction, we're
+ // asking about dereferenceability under the assumption that the
+ // instruction has been speculated to the point of the context instruction,
+ // in which case we don't know if the dereferenceability info still holds.
+ // We don't bother handling allocas here, as they aren't speculatable
+ // anyway.
+ auto *I = dyn_cast<Instruction>(V);
+ if (I && !isa<AllocaInst>(I))
+ return CtxI && isValidAssumeForContext(I, CtxI, DT);
return true;
};
if (IsKnownDeref()) {
diff --git a/llvm/lib/Analysis/MemDerefPrinter.cpp b/llvm/lib/Analysis/MemDerefPrinter.cpp
index e858d941435441..68cb8859488f70 100644
--- a/llvm/lib/Analysis/MemDerefPrinter.cpp
+++ b/llvm/lib/Analysis/MemDerefPrinter.cpp
@@ -30,10 +30,10 @@ PreservedAnalyses MemDerefPrinterPass::run(Function &F,
for (auto &I : instructions(F)) {
if (LoadInst *LI = dyn_cast<LoadInst>(&I)) {
Value *PO = LI->getPointerOperand();
- if (isDereferenceablePointer(PO, LI->getType(), DL))
+ if (isDereferenceablePointer(PO, LI->getType(), DL, LI))
Deref.push_back(PO);
if (isDereferenceableAndAlignedPointer(PO, LI->getType(), LI->getAlign(),
- DL))
+ DL, LI))
DerefAndAligned.insert(PO);
}
}
diff --git a/llvm/lib/CodeGen/MachineOperand.cpp b/llvm/lib/CodeGen/MachineOperand.cpp
index 6ee47624f31c54..89d32c3f005e00 100644
--- a/llvm/lib/CodeGen/MachineOperand.cpp
+++ b/llvm/lib/CodeGen/MachineOperand.cpp
@@ -1047,7 +1047,8 @@ bool MachinePointerInfo::isDereferenceable(unsigned Size, LLVMContext &C,
return false;
return isDereferenceableAndAlignedPointer(
- BasePtr, Align(1), APInt(DL.getPointerSizeInBits(), Offset + Size), DL);
+ BasePtr, Align(1), APInt(DL.getPointerSizeInBits(), Offset + Size), DL,
+ dyn_cast<Instruction>(BasePtr));
}
/// getConstantPool - Return a MachinePointerInfo record that refers to the
diff --git a/llvm/test/Transforms/SimplifyCFG/speculate-derefable-load.ll b/llvm/test/Transforms/SimplifyCFG/speculate-derefable-load.ll
index 8c7afa4598bd4b..0138433312ed84 100644
--- a/llvm/test/Transforms/SimplifyCFG/speculate-derefable-load.ll
+++ b/llvm/test/Transforms/SimplifyCFG/speculate-derefable-load.ll
@@ -77,14 +77,17 @@ exit:
ret i64 %res
}
-; FIXME: This is a miscompile.
define i64 @deref_no_hoist(i1 %c, ptr align 8 dereferenceable(8) %p1) {
; CHECK-LABEL: define i64 @deref_no_hoist(
; CHECK-SAME: i1 [[C:%.*]], ptr align 8 dereferenceable(8) [[P1:%.*]]) {
-; CHECK-NEXT: [[ENTRY:.*:]]
-; CHECK-NEXT: [[P2:%.*]] = load ptr, ptr [[P1]], align 8, !align [[META0:![0-9]+]]
+; CHECK-NEXT: [[ENTRY:.*]]:
+; CHECK-NEXT: br i1 [[C]], label %[[IF:.*]], label %[[EXIT:.*]]
+; CHECK: [[IF]]:
+; CHECK-NEXT: [[P2:%.*]] = load ptr, ptr [[P1]], align 8, !dereferenceable [[META0:![0-9]+]], !align [[META0]]
; CHECK-NEXT: [[V:%.*]] = load i64, ptr [[P2]], align 8
-; CHECK-NEXT: [[RES:%.*]] = select i1 [[C]], i64 [[V]], i64 0
+; CHECK-NEXT: br label %[[EXIT]]
+; CHECK: [[EXIT]]:
+; CHECK-NEXT: [[RES:%.*]] = phi i64 [ [[V]], %[[IF]] ], [ 0, %[[ENTRY]] ]
; CHECK-NEXT: ret i64 [[RES]]
;
entry:
|
If a dereferenceability fact is provided through `!dereferenceable`, it may only hold on the given control flow path. When we use `isSafeToSpeculativelyExecute()` to check multiple instructions, we might make use of `!dereferenceable` information that does not hold at the speculation target. This doesn't happen when speculating instructions one by one, because `!dereferenceable` will be dropped while speculating. Fix this by checking whether the instruction with `!dereferenceable` dominates the context instruction. If this is not the case, it means we are speculating, and cannot guarantee that it holds at the speculation target. Fixes llvm#108854.
| // in which case we don't know if the dereferenceability info still holds. | ||
| // We don't bother handling allocas here, as they aren't speculatable | ||
| // anyway. | ||
| auto *I = dyn_cast<Instruction>(V); |
There was a problem hiding this comment.
if (auto *I = dyn_cast<Instruction>(V))
if (!isa<AllocaInst>(I))
return CtxI && isValidAssumeForContext(I, CtxI, DT);
There was a problem hiding this comment.
if (auto *I = dyn_cast<Instruction>(V))
if (!isa<AllocaInst>(I))
if (CtxI == nullptr)
return false;
else
return isValidAssumeForContext(I, CtxI, DT);
|
Please update the documentation for isSafeToSpeculativelyExecute() to specify the semantics in the case where the operands of the instruction don't dominate CtxI. |
|
I added some wording to isSafeToSpeculativelyExecute(), but not sure if this is what you had in mind. |
|
Yes, that makes sense, thanks. |
|
Is it even possible for |
Yes, it's possible for anything where the derefability is not context-sensitive (like globals, dereferenceable arguments, allocas, etc). |
|
@nikic I mean not in theory but currently. I don't see those (except allocas) handled anywhere unless I'm missing something? |
The |
|
@nikic I see. Thanks. Probably not going to help me come up with a reproducer for a weird behavior/bug I was seeing with LoopVectorizer where it produced loads from poison pointers when it thought it didn't need to predicate a load (after this patch it always needs to since loads are never safe to speculate in its check but I think it's just masking whatever is going on). |
|
@nikic By the way, are there plans to support allocation functions other than alloca in this check? I don't see currently any llvm passes assigning dereferenceable(_or_null) attribute to something like |
I believe we don't mark allocation return values as dereferencable because it would imply a too strong property right now (staying dereferenceable even after the allocation was freed). |
|
@nikic ah, yeah. I remember the issue with dereferenceable w.r.t. free discussion. Guess will have to roll something for allocations that don't have this problem. |
|
@nikic one thing I've discovered with this fact is that now |
|
@danilaml We could pass AllowEphemerals=true to isValidAssumeForContext. |
|
@nikic Does |
|
@nikic also, another situation the current impl doesn't seem to account for is IR like bb1:
%p1 = load dereferenceable(N)
...
bb2:
%p2 = load dereferenceable(M)
...
common:
%p = phi ptr [%p1, %bb1], [%p2, %bb2]
%res = load i32 %p ; <-- dereferenceable(min(N,M)) at least |
6 participants
If a dereferenceability fact is provided through
!dereferenceable(or similar), it may only hold on the given control flow path. When we useisSafeToSpeculativelyExecute()to check multiple instructions, we might make use of!dereferenceableinformation that does not hold at the speculation target. This doesn't happen when speculating instructions one by one, because!dereferenceablewill be dropped while speculating.Fix this by checking whether the instruction with
!dereferenceabledominates the context instruction. If this is not the case, it means we are speculating, and cannot guarantee that it holds at the speculation target.Fixes #108854.