[clang] [llvm] [Loads] Check context instruction for context-sensitive derefability (PR #109277)
danilaml wrote: @nikic also, another situation the current impl doesn't seem to account for is IR like ```llvm 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 ``` https://github.com/llvm/llvm-project/pull/109277 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [llvm] [Loads] Check context instruction for context-sensitive derefability (PR #109277)
danilaml wrote: @nikic Does `AllowEphemerals` actually do what we want in that case? Haven't looked in detail, but on the surface it seemed way more involved than just `if (I == CtxI) return true;`. https://github.com/llvm/llvm-project/pull/109277 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [llvm] [Loads] Check context instruction for context-sensitive derefability (PR #109277)
nikic wrote: @danilaml We could pass AllowEphemerals=true to isValidAssumeForContext. https://github.com/llvm/llvm-project/pull/109277 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [llvm] [Loads] Check context instruction for context-sensitive derefability (PR #109277)
danilaml wrote: @nikic one thing I've discovered with this fact is that now `load` is not dereferenceable at "load" itself due to `isValidAssumeForContext` quirk of not allowing `assume` affect itself. Not sure if it's intentional for attributes here - makes it look like it's not safe to speculate load in its current position. There is also some friction with instruction vs iterator (i.e. when you only have insertion point at the end of bb so can't easily turn it into an instruction), but that's a different matter. https://github.com/llvm/llvm-project/pull/109277 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [llvm] [Loads] Check context instruction for context-sensitive derefability (PR #109277)
nikic wrote: > @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 `malloc(42)` , but I > don't see why not and in that case this should also be something not reliant > on the context. 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). https://github.com/llvm/llvm-project/pull/109277 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [llvm] [Loads] Check context instruction for context-sensitive derefability (PR #109277)
danilaml wrote: @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. https://github.com/llvm/llvm-project/pull/109277 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [llvm] [Loads] Check context instruction for context-sensitive derefability (PR #109277)
danilaml wrote: @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 `malloc(42)` , but I don't see why not and in that case this should also be something not reliant on the context. https://github.com/llvm/llvm-project/pull/109277 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [llvm] [Loads] Check context instruction for context-sensitive derefability (PR #109277)
nikic wrote: > @nikic I mean not in theory but currently. I don't see those (except allocas) > handled anywhere unless I'm missing something? The `I &&` part handles those. If it's a global or argument (thus not an instruction) we'll fall through to the return true. https://github.com/llvm/llvm-project/pull/109277 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [llvm] [Loads] Check context instruction for context-sensitive derefability (PR #109277)
danilaml wrote: @nikic I mean not in theory but currently. I don't see those (except allocas) handled anywhere unless I'm missing something? https://github.com/llvm/llvm-project/pull/109277 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [llvm] [Loads] Check context instruction for context-sensitive derefability (PR #109277)
nikic wrote: > Is it even possible for `isSafeToSpeculativelyExecute` to return `true` now > with the default (nullptr) CtxI for loads? Can > `isDereferenceableAndAlignedPointer` just short-circuit to `false` with null > CtxI? Yes, it's possible for anything where the derefability is not context-sensitive (like globals, dereferenceable arguments, allocas, etc). https://github.com/llvm/llvm-project/pull/109277 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [llvm] [Loads] Check context instruction for context-sensitive derefability (PR #109277)
danilaml wrote: Is it even possible for `isSafeToSpeculativelyExecute` to return `true` now with the default (nullptr) CtxI? https://github.com/llvm/llvm-project/pull/109277 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [llvm] [Loads] Check context instruction for context-sensitive derefability (PR #109277)
https://github.com/nikic closed https://github.com/llvm/llvm-project/pull/109277 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [llvm] [Loads] Check context instruction for context-sensitive derefability (PR #109277)
https://github.com/dtcxzyw approved this pull request. Nice catch! https://github.com/llvm/llvm-project/pull/109277 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [llvm] [Loads] Check context instruction for context-sensitive derefability (PR #109277)
efriedma-quic wrote: Yes, that makes sense, thanks. https://github.com/llvm/llvm-project/pull/109277 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [llvm] [Loads] Check context instruction for context-sensitive derefability (PR #109277)
nikic wrote: I added some wording to isSafeToSpeculativelyExecute(), but not sure if this is what you had in mind. https://github.com/llvm/llvm-project/pull/109277 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [llvm] [Loads] Check context instruction for context-sensitive derefability (PR #109277)
https://github.com/nikic updated
https://github.com/llvm/llvm-project/pull/109277
>From edbdc039ee955cc9d5f0f7d4cb4be287c55e25bb Mon Sep 17 00:00:00 2001
From: Nikita Popov
Date: Tue, 17 Sep 2024 15:48:42 +0200
Subject: [PATCH 1/2] [Loads] Check context instruction for context-sensitive
derefability
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 https://github.com/llvm/llvm-project/issues/108854.
---
clang/test/CodeGenOpenCL/builtins-amdgcn.cl | 6 +-
llvm/lib/Analysis/Loads.cpp | 11 +++
llvm/lib/Analysis/MemDerefPrinter.cpp | 4 ++--
llvm/lib/CodeGen/MachineOperand.cpp | 3 ++-
.../SimplifyCFG/speculate-derefable-load.ll | 11 +++
5 files changed, 23 insertions(+), 12 deletions(-)
diff --git a/clang/test/CodeGenOpenCL/builtins-amdgcn.cl
b/clang/test/CodeGenOpenCL/builtins-amdgcn.cl
index 6a6d5b1dfed3df..9274c80abd8c04 100644
--- a/clang/test/CodeGenOpenCL/builtins-amdgcn.cl
+++ b/clang/test/CodeGenOpenCL/builtins-amdgcn.cl
@@ -638,11 +638,7 @@ void test_get_workgroup_size(int d, global int *out)
// CHECK-LABEL: @test_get_grid_size(
// CHECK: {{.*}}call align 4 dereferenceable(64){{.*}} ptr addrspace(4)
@llvm.amdgcn.dispatch.ptr()
-// CHECK: getelementptr inbounds i8, ptr addrspace(4) %{{.*}}, i64 12
-// CHECK: load i32, ptr addrspace(4) %{{.*}}, align 4, !invariant.load
-// CHECK: getelementptr inbounds i8, ptr addrspace(4) %{{.*}}, i64 16
-// CHECK: load i32, ptr addrspace(4) %{{.*}}, align 4, !invariant.load
-// CHECK: getelementptr inbounds i8, ptr addrspace(4) %{{.*}}, i64 20
+// CHECK: getelementptr inbounds i8, ptr addrspace(4) %{{.*}}, i64 %.sink
// CHECK: load i32, ptr addrspace(4) %{{.*}}, align 4, !invariant.load
void test_get_grid_size(int d, global int *out)
{
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(V);
+if (I && !isa(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(&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(BasePtr));
}
/// getConstantPool - Return a MachinePointerInfo record that refers to the
diff --git a/llvm/test/Transforms/SimplifyCFG/speculate
[clang] [llvm] [Loads] Check context instruction for context-sensitive derefability (PR #109277)
efriedma-quic wrote: Please update the documentation for isSafeToSpeculativelyExecute() to specify the semantics in the case where the operands of the instruction don't dominate CtxI. https://github.com/llvm/llvm-project/pull/109277 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [llvm] [Loads] Check context instruction for context-sensitive derefability (PR #109277)
@@ -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(V); tschuett wrote: ``` if (auto *I = dyn_cast(V)) if (!isa(I)) if (CtxI == nullptr) return false; else return isValidAssumeForContext(I, CtxI, DT); ``` https://github.com/llvm/llvm-project/pull/109277 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [llvm] [Loads] Check context instruction for context-sensitive derefability (PR #109277)
@@ -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(V); tschuett wrote: ``` if (auto *I = dyn_cast(V)) if (!isa(I)) return CtxI && isValidAssumeForContext(I, CtxI, DT); ``` https://github.com/llvm/llvm-project/pull/109277 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [llvm] [Loads] Check context instruction for context-sensitive derefability (PR #109277)
https://github.com/nikic updated
https://github.com/llvm/llvm-project/pull/109277
>From edbdc039ee955cc9d5f0f7d4cb4be287c55e25bb Mon Sep 17 00:00:00 2001
From: Nikita Popov
Date: Tue, 17 Sep 2024 15:48:42 +0200
Subject: [PATCH] [Loads] Check context instruction for context-sensitive
derefability
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 https://github.com/llvm/llvm-project/issues/108854.
---
clang/test/CodeGenOpenCL/builtins-amdgcn.cl | 6 +-
llvm/lib/Analysis/Loads.cpp | 11 +++
llvm/lib/Analysis/MemDerefPrinter.cpp | 4 ++--
llvm/lib/CodeGen/MachineOperand.cpp | 3 ++-
.../SimplifyCFG/speculate-derefable-load.ll | 11 +++
5 files changed, 23 insertions(+), 12 deletions(-)
diff --git a/clang/test/CodeGenOpenCL/builtins-amdgcn.cl
b/clang/test/CodeGenOpenCL/builtins-amdgcn.cl
index 6a6d5b1dfed3df..9274c80abd8c04 100644
--- a/clang/test/CodeGenOpenCL/builtins-amdgcn.cl
+++ b/clang/test/CodeGenOpenCL/builtins-amdgcn.cl
@@ -638,11 +638,7 @@ void test_get_workgroup_size(int d, global int *out)
// CHECK-LABEL: @test_get_grid_size(
// CHECK: {{.*}}call align 4 dereferenceable(64){{.*}} ptr addrspace(4)
@llvm.amdgcn.dispatch.ptr()
-// CHECK: getelementptr inbounds i8, ptr addrspace(4) %{{.*}}, i64 12
-// CHECK: load i32, ptr addrspace(4) %{{.*}}, align 4, !invariant.load
-// CHECK: getelementptr inbounds i8, ptr addrspace(4) %{{.*}}, i64 16
-// CHECK: load i32, ptr addrspace(4) %{{.*}}, align 4, !invariant.load
-// CHECK: getelementptr inbounds i8, ptr addrspace(4) %{{.*}}, i64 20
+// CHECK: getelementptr inbounds i8, ptr addrspace(4) %{{.*}}, i64 %.sink
// CHECK: load i32, ptr addrspace(4) %{{.*}}, align 4, !invariant.load
void test_get_grid_size(int d, global int *out)
{
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(V);
+if (I && !isa(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(&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(BasePtr));
}
/// getConstantPool - Return a MachinePointerInfo record that refers to the
diff --git a/llvm/test/Transforms/SimplifyCFG/speculate-der
