[ValueTracking] Conservative NoSync check prevents vectorization#199002
Conversation
Conservative nosync check in ValueTracking.cpp returns false causing potentially faulting load preventing vectorization. Instead check if any instructions between Assume Instruction and Ctx Instruction are synchronizing.
|
@llvm/pr-subscribers-llvm-transforms @llvm/pr-subscribers-llvm-analysis Author: Kshitij Paranjape (kshitijvp) ChangesConservative nosync check in ValueTracking.cpp returns false causing potentially faulting load preventing vectorization. Instead check if any instructions between Assume Instruction and Ctx Instruction are synchronizing. Full diff: https://github.com/llvm/llvm-project/pull/199002.diff 2 Files Affected:
diff --git a/llvm/include/llvm/Analysis/ValueTracking.h b/llvm/include/llvm/Analysis/ValueTracking.h
index b2f664a9c9c0d..79ff896779ce2 100644
--- a/llvm/include/llvm/Analysis/ValueTracking.h
+++ b/llvm/include/llvm/Analysis/ValueTracking.h
@@ -639,7 +639,7 @@ inline bool isValidAssumeForContext(const Instruction *I,
}
/// Returns true, if no instruction between \p Assume and \p CtxI may free
-/// memory and the function is marked as NoSync. The latter ensures the current
+/// or synchronize memory. The latter ensures the current
/// function cannot arrange for another thread to free on its behalf.
LLVM_ABI bool willNotFreeBetween(const Instruction *Assume,
const Instruction *CtxI);
diff --git a/llvm/lib/Analysis/ValueTracking.cpp b/llvm/lib/Analysis/ValueTracking.cpp
index 3be0db5ad9c7e..b59bed0cc7f23 100644
--- a/llvm/lib/Analysis/ValueTracking.cpp
+++ b/llvm/lib/Analysis/ValueTracking.cpp
@@ -703,14 +703,26 @@ bool llvm::isValidAssumeForContext(const Instruction *Inv,
bool llvm::willNotFreeBetween(const Instruction *Assume,
const Instruction *CtxI) {
- // Helper to check if there are any calls in the range that may free memory.
+ // Helper to make sure the current function cannot arrange for
+ // another thread to free on its behalf and to check if there
+ // are any calls in the range that may free memory.
auto hasNoFreeCalls = [](auto Range) {
for (const auto &[Idx, I] : enumerate(Range)) {
if (Idx > MaxInstrsToCheckForFree)
return false;
- if (const auto *CB = dyn_cast<CallBase>(&I))
- if (!CB->hasFnAttr(Attribute::NoFree))
- return false;
+ if (I.isVolatile())
+ return false;
+
+ if (I.maySynchronize())
+ return false;
+
+ auto *CB = dyn_cast<CallBase>(&I);
+ if (CB) {
+ // Non call site cases covered by the two checks above
+ if (!CB->hasFnAttr(Attribute::NoSync) ||
+ !CB->hasFnAttr(Attribute::NoFree))
+ return false;
+ }
}
return true;
};
@@ -739,7 +751,7 @@ bool llvm::willNotFreeBetween(const Instruction *Assume,
}
// Check if there are any calls between Assume and CtxIter that may free
- // memory.
+ // memory or synchronize.
return hasNoFreeCalls(make_range(Assume->getIterator(), CtxIter));
}
|
|
✅ With the latest revision this PR passed the C/C++ code formatter. |
|
I will add the cleaned IR Tests soon. |
fhahn
left a comment
There was a problem hiding this comment.
Is there a reason to re-posting this as a new PR again instead of just updating the existing one? It is very hard to keep track of the comments
There was some problem with the previous PR and Nikic wasn't able to add reviews so had to open a new one. |
|
Since Nikic pointed out, instead of isOrderedAtomic I have used I.maySynchronize(), and have also used the same name since synchronization is just another way of freeing. |
|
I have added the cleaned IR Tests. |
🪟 Windows x64 Test Results
✅ The build succeeded and all tests passed. |
🐧 Linux x64 Test Results
✅ The build succeeded and all tests passed. |
nikic
left a comment
There was a problem hiding this comment.
Sorry, one more semantics change... otherwise this looks good to me.
| call void @llvm.assume(i1 true) [ "align"(ptr %l.gep.v, i64 4) ] | ||
| %pi.l.gep.v = ptrtoint ptr %l.gep.v to i64 | ||
| %sub = sub i64 %pi.l.gep.v, %pi.l.v | ||
| call void @llvm.assume(i1 true) [ "dereferenceable"(ptr %l.v, i64 %sub) ] |
There was a problem hiding this comment.
The file focuses on testing deref assumptions with constant sizes. With non-constant expressions should go to llvm/test/Transforms/LoopVectorize/dereferenceable-info-from-assumption-variable-size.ll
But the interesting aspect of the new tests are the nosync etc checks. so I think you can just remove the ptrtoint/sub code to compute the deref size. Similarly, you can just pass l.v and l.gep.v as pointer arguments without loss of generality.
There was a problem hiding this comment.
On removing those ptrtoint/sub the Tests which should be vectorized are not getting vectorized.
I will just move the tests to
llvm/test/Transforms/LoopVectorize/dereferenceable-info-from-assumption-variable-size.ll
There was a problem hiding this comment.
Right, but I think it should be possible to take one of the existing simpler tests that do not get vectorized from llvm/test/Transforms/LoopVectorize/dereferenceable-info-from-assumption-constant-size.ll like the changed deref_assumption_in_header_constant_trip_count_nofree_via_context_b and use that to test the main logic skipping with various instructions before the assumption? That way the entry block would be a single deref assumption + the instruction we are testing.
The existing positive test with the early exit could go llvm/test/Transforms/LoopVectorize/single-early-exit-deref-assumptions.ll, I forgot that there was a dedicated file for reasoning about early exits
There was a problem hiding this comment.
@fhahn Isn't deref_assumption_in_header_constant_trip_count_nofree_via_context_but_missing_nosync getting vectorized too?
The Check-Lines contain the Vector-Preheader and Vector-Body which means it is getting vectorized.
There was a problem hiding this comment.
I attempted rewrote it to not get vectorized by creating a new point %a.1 which loads from %a.
And an atomic instruction that works on %a next to the dereferenceable instruction. The Loop Ends up using that %a.
However still that is getting vectorized.
There was a problem hiding this comment.
All of the tests within that file are getting vectorized.
There was a problem hiding this comment.
Yep, what I meant is take deref_assumption_in_header_constant_trip_count_nofree_via_context_but_missing_nosync and add the volatile loads etc
There was a problem hiding this comment.
define void @atomicNotVectorizingTest(ptr noalias noundef %a, ptr noalias %b, ptr noalias %c) {
; CHECK-LABEL: define void @atomicNotVectorizingTest(
; CHECK-SAME: ptr noalias noundef [[A:%.*]], ptr noalias [[B:%.*]], ptr noalias [[C:%.*]]) {
; CHECK-NEXT: [[ENTRY:.*:]]
; CHECK-NEXT: [[A_1:%.*]] = load ptr, ptr [[A]], align 8
; CHECK-NEXT: call void @llvm.assume(i1 true) [ "align"(ptr [[A_1]], i64 4), "dereferenceable"(ptr [[A_1]], i64 4000) ]
; CHECK-NEXT: [[ATOMIC:%.*]] = load atomic ptr, ptr [[A]] seq_cst, align 8
; CHECK-NEXT: br label %[[VECTOR_PH:.*]]
; CHECK: [[VECTOR_PH]]:
; CHECK-NEXT: br label %[[PRED_LOAD_IF:.*]]
; CHECK: [[PRED_LOAD_IF]]:
; CHECK-NEXT: [[INDEX:%.*]] = phi i64 [ 0, %[[VECTOR_PH]] ], [ [[INDEX_NEXT:%.*]], %[[PRED_LOAD_IF5:.*]] ]
; CHECK-NEXT: [[TMP5:%.*]] = getelementptr inbounds i32, ptr [[B]], i64 [[INDEX]]
; CHECK-NEXT: [[WIDE_LOAD:%.*]] = load <2 x i32>, ptr [[TMP5]], align 4
; CHECK-NEXT: [[TMP4:%.*]] = icmp slt <2 x i32> [[WIDE_LOAD]], zeroinitializer
; CHECK-NEXT: [[TMP10:%.*]] = extractelement <2 x i1> [[TMP4]], i64 0
; CHECK-NEXT: br i1 [[TMP10]], label %[[PRED_LOAD_IF1:.*]], label %[[PRED_LOAD_CONTINUE:.*]]
; CHECK: [[PRED_LOAD_IF1]]:
; CHECK-NEXT: [[TMP6:%.*]] = getelementptr i32, ptr [[A_1]], i64 [[INDEX]]
; CHECK-NEXT: [[TMP7:%.*]] = load i32, ptr [[TMP6]], align 4
; CHECK-NEXT: [[TMP8:%.*]] = insertelement <2 x i32> poison, i32 [[TMP7]], i64 0
; CHECK-NEXT: br label %[[PRED_LOAD_CONTINUE]]
; CHECK: [[PRED_LOAD_CONTINUE]]:
; CHECK-NEXT: [[TMP9:%.*]] = phi <2 x i32> [ poison, %[[PRED_LOAD_IF]] ], [ [[TMP8]], %[[PRED_LOAD_IF1]] ]
; CHECK-NEXT: [[C_1:%.*]] = extractelement <2 x i1> [[TMP4]], i64 1
; CHECK-NEXT: br i1 [[C_1]], label %[[PRED_LOAD_CONTINUE6:.*]], label %[[PRED_LOAD_IF5]]
; CHECK: [[PRED_LOAD_CONTINUE6]]:
; CHECK-NEXT: [[TMP13:%.*]] = add i64 [[INDEX]], 1
; CHECK-NEXT: [[TMP11:%.*]] = getelementptr i32, ptr [[A_1]], i64 [[TMP13]]
; CHECK-NEXT: [[TMP12:%.*]] = load i32, ptr [[TMP11]], align 4
; CHECK-NEXT: [[TMP14:%.*]] = insertelement <2 x i32> [[TMP9]], i32 [[TMP12]], i64 1
; CHECK-NEXT: br label %[[PRED_LOAD_IF5]]
; CHECK: [[PRED_LOAD_IF5]]:
; CHECK-NEXT: [[TMP16:%.*]] = phi <2 x i32> [ [[TMP9]], %[[PRED_LOAD_CONTINUE]] ], [ [[TMP14]], %[[PRED_LOAD_CONTINUE6]] ]
; CHECK-NEXT: [[PREDPHI:%.*]] = select <2 x i1> [[TMP4]], <2 x i32> [[TMP16]], <2 x i32> [[WIDE_LOAD]]
; CHECK-NEXT: [[TMP15:%.*]] = getelementptr inbounds i32, ptr [[C]], i64 [[INDEX]]
; CHECK-NEXT: store <2 x i32> [[PREDPHI]], ptr [[TMP15]], align 4
; CHECK-NEXT: [[INDEX_NEXT]] = add nuw i64 [[INDEX]], 2
; CHECK-NEXT: [[TMP17:%.*]] = icmp eq i64 [[INDEX_NEXT]], 1000
; CHECK-NEXT: br i1 [[TMP17]], label %[[MIDDLE_BLOCK:.*]], label %[[PRED_LOAD_IF]], !llvm.loop [[LOOP24:![0-9]+]]
; CHECK: [[MIDDLE_BLOCK]]:
; CHECK-NEXT: br label %[[EXIT:.*]]
; CHECK: [[EXIT]]:
; CHECK-NEXT: ret void
;
entry:
%a.1 = load ptr, ptr %a, align 8
call void @llvm.assume(i1 true) [ "align"(ptr %a.1, i64 4), "dereferenceable"(ptr %a.1, i64 4000) ]
%atomic = load atomic ptr, ptr %a seq_cst, align 8 ; Atomic Instruction
br label %loop.header
loop.header:
%iv = phi i64 [ 0, %entry ], [ %iv.next, %loop.latch ]
%gep.a = getelementptr i32, ptr %a.1, i64 %iv
%gep.b = getelementptr inbounds i32, ptr %b, i64 %iv
%l.b = load i32, ptr %gep.b, align 4
%c.1 = icmp sge i32 %l.b, 0
br i1 %c.1, label %loop.latch, label %loop.then
loop.then:
%l.a = load i32, ptr %gep.a, align 4
br label %loop.latch
loop.latch:
%merge = phi i32 [ %l.a, %loop.then ], [ %l.b, %loop.header ]
%gep.c = getelementptr inbounds i32, ptr %c, i64 %iv
store i32 %merge, ptr %gep.c, align 4
%iv.next = add nuw nsw i64 %iv, 1
%ec = icmp eq i64 %iv.next, 1000
br i1 %ec, label %exit, label %loop.header
exit:
ret void
}
Here is my modified test. I have added the Atomic Instruction after the Dereferenceable Asssume Instruction. However still this is getting Vectorized...
Putting the Atomic Instruction inside the Loop Header stops vectorization but that's not what we want to test right?
There was a problem hiding this comment.
yes it vectorizes, but does not execute the load in loop.then unconditionally. If the deref assumption can be used, it will execute the load unconditionally.
So with and without the load atomic it will get vectorized, but differently
There was a problem hiding this comment.
I have made the changes.
|
Why is this Test Failing? |
|
@kshitijvp Tests run against main, it's possible that something in the meantime affected the test output. You can try whether merging main reproduces the issue for you. |
Added checks in NoSync lambda function to account for other synchronizing instructions such as fence, volatile, atomic instructions. Also added more negative tests that do not get vectorized when they contain synchronizing instructions between assume instruction and CtxI instruction.
|
@fhahn Can I merge? |
…m#199002) Conservative nosync check in ValueTracking.cpp returns false causing potentially faulting load preventing vectorization. Instead check if any instructions betweenAssume Instruction and Ctx Instruction are synchronizing. Fixes llvm#180180.
Conservative nosync check in ValueTracking.cpp returns false causing potentially
faulting load preventing vectorization. Instead check if any instructions between
Assume Instruction and Ctx Instruction are synchronizing.
Fixes #180180