[LoopVersioningLICM] Only mark pointers with generated checks as noalias #135168
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Currently when we version a loop all loads and stores have the noalias metadata added to them. If there were some pointers that could not be analyzed, and thus we could not generate runtime aliasing checks for, then we should not mark loads and stores using these pointers as noalias. Currently this does nothing, as LoopAccessAnalysis discards all results if it couldn't analyse every pointer leading to no loop ersioning happening, but an upcoming patch will change that and we need this first otherwise we incorrectly mark some pointers as noalias even when they aren't.
|
@llvm/pr-subscribers-llvm-transforms Author: John Brawn (john-brawn-arm) ChangesCurrently when we version a loop all loads and stores have the noalias metadata added to them. If there were some pointers that could not be analysed, and thus we could not generate runtime aliasing checks for, then we should not mark loads and stores using these pointers as noalias. Currently this does nothing, as LoopAccessAnalysis discards all results if it couldn't analyse every pointer leading to no loop versioning happening, but an upcoming patch will change that and we need this first otherwise we incorrectly mark some pointers as noalias even when they aren't. Full diff: https://github.com/llvm/llvm-project/pull/135168.diff 2 Files Affected:
diff --git a/llvm/lib/Transforms/Scalar/LoopVersioningLICM.cpp b/llvm/lib/Transforms/Scalar/LoopVersioningLICM.cpp
index 6e91c4fa6e230..74b424655a9fa 100644
--- a/llvm/lib/Transforms/Scalar/LoopVersioningLICM.cpp
+++ b/llvm/lib/Transforms/Scalar/LoopVersioningLICM.cpp
@@ -344,6 +344,13 @@ bool LoopVersioningLICM::instructionSafeForVersioning(Instruction *I) {
}
LoadAndStoreCounter++;
Value *Ptr = St->getPointerOperand();
+ // Don't allow stores that we don't have runtime checks for, as we won't be
+ // able to mark them noalias meaning they would prevent any code motion.
+ auto &Pointers = LAI->getRuntimePointerChecking()->Pointers;
+ if (!any_of(Pointers, [&](auto &P) { return P.PointerValue == Ptr; })) {
+ LLVM_DEBUG(dbgs() << " Found a store without a runtime check.\n");
+ return false;
+ }
// Check loop invariant.
if (SE->isLoopInvariant(SE->getSCEV(Ptr), CurLoop))
InvariantCounter++;
@@ -361,6 +368,13 @@ bool LoopVersioningLICM::legalLoopInstructions() {
InvariantCounter = 0;
IsReadOnlyLoop = true;
using namespace ore;
+ // Get LoopAccessInfo from current loop via the proxy.
+ LAI = &LAIs.getInfo(*CurLoop);
+ // Check LoopAccessInfo for need of runtime check.
+ if (LAI->getRuntimePointerChecking()->getChecks().empty()) {
+ LLVM_DEBUG(dbgs() << " LAA: Runtime check not found !!\n");
+ return false;
+ }
// Iterate over loop blocks and instructions of each block and check
// instruction safety.
for (auto *Block : CurLoop->getBlocks())
@@ -374,13 +388,6 @@ bool LoopVersioningLICM::legalLoopInstructions() {
return false;
}
}
- // Get LoopAccessInfo from current loop via the proxy.
- LAI = &LAIs.getInfo(*CurLoop);
- // Check LoopAccessInfo for need of runtime check.
- if (LAI->getRuntimePointerChecking()->getChecks().empty()) {
- LLVM_DEBUG(dbgs() << " LAA: Runtime check not found !!\n");
- return false;
- }
// Number of runtime-checks should be less then RuntimeMemoryCheckThreshold
if (LAI->getNumRuntimePointerChecks() >
VectorizerParams::RuntimeMemoryCheckThreshold) {
@@ -515,13 +522,16 @@ void LoopVersioningLICM::setNoAliasToLoop(Loop *VerLoop) {
StringRef Name = "LVAliasScope";
MDNode *NewScope = MDB.createAnonymousAliasScope(NewDomain, Name);
SmallVector<Metadata *, 4> Scopes{NewScope}, NoAliases{NewScope};
+ auto &Pointers = LAI->getRuntimePointerChecking()->Pointers;
+
// Iterate over each instruction of loop.
// set no-alias for all load & store instructions.
for (auto *Block : CurLoop->getBlocks()) {
for (auto &Inst : *Block) {
- // Only interested in instruction that may modify or read memory.
- if (!Inst.mayReadFromMemory() && !Inst.mayWriteToMemory())
- continue;
+ // We can only add noalias to pointers that we've inserted checks for
+ Value *V = getLoadStorePointerOperand(&Inst);
+ if (!V || !any_of(Pointers, [&](auto &P) { return P.PointerValue == V; }))
+ continue;
// Set no-alias for current instruction.
Inst.setMetadata(
LLVMContext::MD_noalias,
diff --git a/llvm/test/Transforms/LoopVersioningLICM/load-from-unknown-address.ll b/llvm/test/Transforms/LoopVersioningLICM/load-from-unknown-address.ll
new file mode 100644
index 0000000000000..e9b2954039198
--- /dev/null
+++ b/llvm/test/Transforms/LoopVersioningLICM/load-from-unknown-address.ll
@@ -0,0 +1,307 @@
+; NOTE: Assertions have been autogenerated by utils/update_test_checks.py UTC_ARGS: --check-globals none --version 5
+; RUN: opt < %s -S -passes='function(loop-versioning-licm,loop-mssa(licm))' | FileCheck %s
+
+target datalayout = "e-m:e-p270:32:32-p271:32:32-p272:64:64-i8:8:32-i16:16:32-i64:64-i128:128-n32:64-S128-Fn32"
+
+; In these tests we have a loop where we can calculate the bounds of some memory
+; accesses but not others.
+
+; Load from a gep whose bounds can't be calculated as the offset is loaded from memory
+; FIXME: Not knowing the bounds of the gep shouldn't stop us from hoisting the load of rval
+define void @gep_loaded_offset(ptr %p, ptr %q, ptr %r, i32 %n) {
+; CHECK-LABEL: define void @gep_loaded_offset(
+; CHECK-SAME: ptr [[P:%.*]], ptr [[Q:%.*]], ptr [[R:%.*]], i32 [[N:%.*]]) {
+; CHECK-NEXT: [[ENTRY:.*]]:
+; CHECK-NEXT: br label %[[WHILE_BODY:.*]]
+; CHECK: [[WHILE_BODY]]:
+; CHECK-NEXT: [[N_ADDR:%.*]] = phi i32 [ [[DEC:%.*]], %[[WHILE_BODY]] ], [ [[N]], %[[ENTRY]] ]
+; CHECK-NEXT: [[P_ADDR:%.*]] = phi ptr [ [[INCDEC_PTR:%.*]], %[[WHILE_BODY]] ], [ [[P]], %[[ENTRY]] ]
+; CHECK-NEXT: [[DEC]] = add nsw i32 [[N_ADDR]], -1
+; CHECK-NEXT: [[RVAL:%.*]] = load i64, ptr [[R]], align 4
+; CHECK-NEXT: [[ARRAYIDX:%.*]] = getelementptr inbounds i32, ptr [[Q]], i64 [[RVAL]]
+; CHECK-NEXT: [[VAL:%.*]] = load i32, ptr [[ARRAYIDX]], align 4
+; CHECK-NEXT: [[INCDEC_PTR]] = getelementptr inbounds nuw i8, ptr [[P_ADDR]], i64 4
+; CHECK-NEXT: store i32 [[VAL]], ptr [[P_ADDR]], align 4
+; CHECK-NEXT: [[TOBOOL_NOT:%.*]] = icmp eq i32 [[DEC]], 0
+; CHECK-NEXT: br i1 [[TOBOOL_NOT]], label %[[WHILE_END:.*]], label %[[WHILE_BODY]]
+; CHECK: [[WHILE_END]]:
+; CHECK-NEXT: ret void
+;
+entry:
+ br label %while.body
+
+while.body:
+ %n.addr = phi i32 [ %dec, %while.body ], [ %n, %entry ]
+ %p.addr = phi ptr [ %incdec.ptr, %while.body ], [ %p, %entry ]
+ %dec = add nsw i32 %n.addr, -1
+ %rval = load i64, ptr %r, align 4
+ %arrayidx = getelementptr inbounds i32, ptr %q, i64 %rval
+ %val = load i32, ptr %arrayidx, align 4
+ %incdec.ptr = getelementptr inbounds nuw i8, ptr %p.addr, i64 4
+ store i32 %val, ptr %p.addr, align 4
+ %tobool.not = icmp eq i32 %dec, 0
+ br i1 %tobool.not, label %while.end, label %while.body
+
+while.end:
+ ret void
+}
+
+; As above but with a store to the loaded address. This should prevent the loop
+; from being versioned, as we wouldn't be able to do any code motion.
+define void @gep_loaded_offset_with_store(ptr %p, ptr %q, ptr %r, i32 %n) {
+; CHECK-LABEL: define void @gep_loaded_offset_with_store(
+; CHECK-SAME: ptr [[P:%.*]], ptr [[Q:%.*]], ptr [[R:%.*]], i32 [[N:%.*]]) {
+; CHECK-NEXT: [[ENTRY:.*]]:
+; CHECK-NEXT: br label %[[WHILE_BODY:.*]]
+; CHECK: [[WHILE_BODY]]:
+; CHECK-NEXT: [[N_ADDR:%.*]] = phi i32 [ [[DEC:%.*]], %[[WHILE_BODY]] ], [ [[N]], %[[ENTRY]] ]
+; CHECK-NEXT: [[P_ADDR:%.*]] = phi ptr [ [[INCDEC_PTR:%.*]], %[[WHILE_BODY]] ], [ [[P]], %[[ENTRY]] ]
+; CHECK-NEXT: [[DEC]] = add nsw i32 [[N_ADDR]], -1
+; CHECK-NEXT: [[RVAL:%.*]] = load i64, ptr [[R]], align 4
+; CHECK-NEXT: [[ARRAYIDX:%.*]] = getelementptr inbounds i32, ptr [[Q]], i64 [[RVAL]]
+; CHECK-NEXT: [[VAL:%.*]] = load i32, ptr [[ARRAYIDX]], align 4
+; CHECK-NEXT: store i32 0, ptr [[ARRAYIDX]], align 4
+; CHECK-NEXT: [[INCDEC_PTR]] = getelementptr inbounds nuw i8, ptr [[P_ADDR]], i64 4
+; CHECK-NEXT: store i32 [[VAL]], ptr [[P_ADDR]], align 4
+; CHECK-NEXT: [[TOBOOL_NOT:%.*]] = icmp eq i32 [[DEC]], 0
+; CHECK-NEXT: br i1 [[TOBOOL_NOT]], label %[[WHILE_END:.*]], label %[[WHILE_BODY]]
+; CHECK: [[WHILE_END]]:
+; CHECK-NEXT: ret void
+;
+entry:
+ br label %while.body
+
+while.body:
+ %n.addr = phi i32 [ %dec, %while.body ], [ %n, %entry ]
+ %p.addr = phi ptr [ %incdec.ptr, %while.body ], [ %p, %entry ]
+ %dec = add nsw i32 %n.addr, -1
+ %rval = load i64, ptr %r, align 4
+ %arrayidx = getelementptr inbounds i32, ptr %q, i64 %rval
+ %val = load i32, ptr %arrayidx, align 4
+ store i32 0, ptr %arrayidx, align 4
+ %incdec.ptr = getelementptr inbounds nuw i8, ptr %p.addr, i64 4
+ store i32 %val, ptr %p.addr, align 4
+ %tobool.not = icmp eq i32 %dec, 0
+ br i1 %tobool.not, label %while.end, label %while.body
+
+while.end:
+ ret void
+}
+
+; Load from a gep whose bounds can't be calculated as the pointer is loaded from memory
+; FIXME: Not knowing the bounds of the gep shouldn't stop us from hoisting the load of rval
+define void @gep_loaded_base(ptr %p, ptr %q, ptr %r, i32 %n) {
+; CHECK-LABEL: define void @gep_loaded_base(
+; CHECK-SAME: ptr [[P:%.*]], ptr [[Q:%.*]], ptr [[R:%.*]], i32 [[N:%.*]]) {
+; CHECK-NEXT: [[ENTRY:.*]]:
+; CHECK-NEXT: br label %[[WHILE_BODY:.*]]
+; CHECK: [[WHILE_BODY]]:
+; CHECK-NEXT: [[N_ADDR:%.*]] = phi i32 [ [[DEC:%.*]], %[[WHILE_BODY]] ], [ [[N]], %[[ENTRY]] ]
+; CHECK-NEXT: [[P_ADDR:%.*]] = phi ptr [ [[INCDEC_PTR:%.*]], %[[WHILE_BODY]] ], [ [[P]], %[[ENTRY]] ]
+; CHECK-NEXT: [[DEC]] = add nsw i32 [[N_ADDR]], -1
+; CHECK-NEXT: [[RVAL:%.*]] = load ptr, ptr [[R]], align 4
+; CHECK-NEXT: [[ARRAYIDX:%.*]] = getelementptr inbounds i32, ptr [[RVAL]], i64 0
+; CHECK-NEXT: [[VAL:%.*]] = load i32, ptr [[ARRAYIDX]], align 4
+; CHECK-NEXT: [[INCDEC_PTR]] = getelementptr inbounds nuw i8, ptr [[P_ADDR]], i64 4
+; CHECK-NEXT: store i32 [[VAL]], ptr [[P_ADDR]], align 4
+; CHECK-NEXT: [[TOBOOL_NOT:%.*]] = icmp eq i32 [[DEC]], 0
+; CHECK-NEXT: br i1 [[TOBOOL_NOT]], label %[[WHILE_END:.*]], label %[[WHILE_BODY]]
+; CHECK: [[WHILE_END]]:
+; CHECK-NEXT: ret void
+;
+entry:
+ br label %while.body
+
+while.body:
+ %n.addr = phi i32 [ %dec, %while.body ], [ %n, %entry ]
+ %p.addr = phi ptr [ %incdec.ptr, %while.body ], [ %p, %entry ]
+ %dec = add nsw i32 %n.addr, -1
+ %rval = load ptr, ptr %r, align 4
+ %arrayidx = getelementptr inbounds i32, ptr %rval, i64 0
+ %val = load i32, ptr %arrayidx, align 4
+ %incdec.ptr = getelementptr inbounds nuw i8, ptr %p.addr, i64 4
+ store i32 %val, ptr %p.addr, align 4
+ %tobool.not = icmp eq i32 %dec, 0
+ br i1 %tobool.not, label %while.end, label %while.body
+
+while.end:
+ ret void
+}
+
+; Load from a gep with an offset that scalar evolution can't describe
+; FIXME: Not knowing the bounds of the gep shouldn't stop us from hoisting the load of qval
+define void @gep_strange_offset(ptr %p, ptr %q, ptr %r, i32 %n) {
+; CHECK-LABEL: define void @gep_strange_offset(
+; CHECK-SAME: ptr [[P:%.*]], ptr [[Q:%.*]], ptr [[R:%.*]], i32 [[N:%.*]]) {
+; CHECK-NEXT: [[ENTRY:.*]]:
+; CHECK-NEXT: br label %[[WHILE_BODY:.*]]
+; CHECK: [[WHILE_BODY]]:
+; CHECK-NEXT: [[N_ADDR:%.*]] = phi i32 [ [[DEC:%.*]], %[[WHILE_BODY]] ], [ [[N]], %[[ENTRY]] ]
+; CHECK-NEXT: [[P_ADDR:%.*]] = phi ptr [ [[INCDEC_PTR:%.*]], %[[WHILE_BODY]] ], [ [[P]], %[[ENTRY]] ]
+; CHECK-NEXT: [[DEC]] = add nsw i32 [[N_ADDR]], -1
+; CHECK-NEXT: [[QVAL:%.*]] = load i32, ptr [[Q]], align 4
+; CHECK-NEXT: [[REM:%.*]] = srem i32 [[DEC]], 2
+; CHECK-NEXT: [[IDXPROM:%.*]] = sext i32 [[REM]] to i64
+; CHECK-NEXT: [[ARRAYIDX:%.*]] = getelementptr inbounds i32, ptr [[R]], i64 [[IDXPROM]]
+; CHECK-NEXT: [[VAL:%.*]] = load i32, ptr [[ARRAYIDX]], align 4
+; CHECK-NEXT: [[ADD:%.*]] = add nsw i32 [[VAL]], [[QVAL]]
+; CHECK-NEXT: [[INCDEC_PTR]] = getelementptr inbounds nuw i8, ptr [[P_ADDR]], i64 4
+; CHECK-NEXT: store i32 [[ADD]], ptr [[P_ADDR]], align 4
+; CHECK-NEXT: [[TOBOOL_NOT:%.*]] = icmp eq i32 [[DEC]], 0
+; CHECK-NEXT: br i1 [[TOBOOL_NOT]], label %[[WHILE_END:.*]], label %[[WHILE_BODY]]
+; CHECK: [[WHILE_END]]:
+; CHECK-NEXT: ret void
+;
+entry:
+ br label %while.body
+
+while.body:
+ %n.addr = phi i32 [ %dec, %while.body ], [ %n, %entry ]
+ %p.addr = phi ptr [ %incdec.ptr, %while.body ], [ %p, %entry ]
+ %dec = add nsw i32 %n.addr, -1
+ %qval = load i32, ptr %q, align 4
+ %rem = srem i32 %dec, 2
+ %idxprom = sext i32 %rem to i64
+ %arrayidx = getelementptr inbounds i32, ptr %r, i64 %idxprom
+ %val = load i32, ptr %arrayidx, align 4
+ %add = add nsw i32 %val, %qval
+ %incdec.ptr = getelementptr inbounds nuw i8, ptr %p.addr, i64 4
+ store i32 %add, ptr %p.addr, align 4
+ %tobool.not = icmp eq i32 %dec, 0
+ br i1 %tobool.not, label %while.end, label %while.body
+
+while.end:
+ ret void
+}
+
+; A memcpy-like loop where the source address is loaded from a pointer
+; FIXME: We should be able to hoist the load of the source address pointer
+define void @memcpy_load_src(ptr %dst, ptr %src, i32 %n) {
+; CHECK-LABEL: define void @memcpy_load_src(
+; CHECK-SAME: ptr [[DST:%.*]], ptr [[SRC:%.*]], i32 [[N:%.*]]) {
+; CHECK-NEXT: [[ENTRY:.*]]:
+; CHECK-NEXT: br label %[[WHILE_BODY:.*]]
+; CHECK: [[WHILE_BODY]]:
+; CHECK-NEXT: [[N_VAL:%.*]] = phi i32 [ [[DEC:%.*]], %[[WHILE_BODY]] ], [ [[N]], %[[ENTRY]] ]
+; CHECK-NEXT: [[DST_VAL:%.*]] = phi ptr [ [[DST_VAL_NEXT:%.*]], %[[WHILE_BODY]] ], [ [[DST]], %[[ENTRY]] ]
+; CHECK-NEXT: [[DEC]] = add nsw i32 [[N_VAL]], -1
+; CHECK-NEXT: [[SRC_VAL:%.*]] = load ptr, ptr [[SRC]], align 8
+; CHECK-NEXT: [[SRC_VAL_NEXT:%.*]] = getelementptr inbounds nuw i8, ptr [[SRC_VAL]], i64 1
+; CHECK-NEXT: [[DST_VAL_NEXT]] = getelementptr inbounds nuw i8, ptr [[DST_VAL]], i64 1
+; CHECK-NEXT: store ptr [[SRC_VAL_NEXT]], ptr [[SRC]], align 8
+; CHECK-NEXT: [[VAL:%.*]] = load i8, ptr [[SRC_VAL]], align 1
+; CHECK-NEXT: store i8 [[VAL]], ptr [[DST_VAL]], align 1
+; CHECK-NEXT: [[TOBOOL_NOT:%.*]] = icmp eq i32 [[DEC]], 0
+; CHECK-NEXT: br i1 [[TOBOOL_NOT]], label %[[WHILE_END:.*]], label %[[WHILE_BODY]]
+; CHECK: [[WHILE_END]]:
+; CHECK-NEXT: ret void
+;
+entry:
+ br label %while.body
+
+while.body:
+ %n_val = phi i32 [ %dec, %while.body ], [ %n, %entry ]
+ %dst_val = phi ptr [ %dst_val.next, %while.body ], [ %dst, %entry ]
+ %dec = add nsw i32 %n_val, -1
+ %src_val = load ptr, ptr %src, align 8
+ %src_val.next = getelementptr inbounds nuw i8, ptr %src_val, i64 1
+ %dst_val.next = getelementptr inbounds nuw i8, ptr %dst_val, i64 1
+ store ptr %src_val.next, ptr %src, align 8
+ %val = load i8, ptr %src_val, align 1
+ store i8 %val, ptr %dst_val, align 1
+ %tobool.not = icmp eq i32 %dec, 0
+ br i1 %tobool.not, label %while.end, label %while.body
+
+while.end:
+ ret void
+}
+
+; A memcpy-like loop where the destination address is loaded from a pointer
+; FIXME: We could hoist the load of the destination address, but doing the
+; bounds check of the store through that pointer itself requires using the
+; hoisted load.
+define void @memcpy_load_dst(ptr %dst, ptr %src, i32 %n) {
+; CHECK-LABEL: define void @memcpy_load_dst(
+; CHECK-SAME: ptr [[DST:%.*]], ptr [[SRC:%.*]], i32 [[N:%.*]]) {
+; CHECK-NEXT: [[ENTRY:.*]]:
+; CHECK-NEXT: br label %[[WHILE_BODY:.*]]
+; CHECK: [[WHILE_BODY]]:
+; CHECK-NEXT: [[N_VAL:%.*]] = phi i32 [ [[DEC:%.*]], %[[WHILE_BODY]] ], [ [[N]], %[[ENTRY]] ]
+; CHECK-NEXT: [[SRC_VAL:%.*]] = phi ptr [ [[SRC_VAL_NEXT:%.*]], %[[WHILE_BODY]] ], [ [[SRC]], %[[ENTRY]] ]
+; CHECK-NEXT: [[DEC]] = add nsw i32 [[N_VAL]], -1
+; CHECK-NEXT: [[DST_VAL:%.*]] = load ptr, ptr [[DST]], align 8
+; CHECK-NEXT: [[SRC_VAL_NEXT]] = getelementptr inbounds nuw i8, ptr [[SRC_VAL]], i64 1
+; CHECK-NEXT: [[DST_VAL_NEXT:%.*]] = getelementptr inbounds nuw i8, ptr [[DST_VAL]], i64 1
+; CHECK-NEXT: store ptr [[DST_VAL_NEXT]], ptr [[DST]], align 8
+; CHECK-NEXT: [[VAL:%.*]] = load i8, ptr [[SRC_VAL]], align 1
+; CHECK-NEXT: store i8 [[VAL]], ptr [[DST_VAL]], align 1
+; CHECK-NEXT: [[TOBOOL_NOT:%.*]] = icmp eq i32 [[DEC]], 0
+; CHECK-NEXT: br i1 [[TOBOOL_NOT]], label %[[WHILE_END:.*]], label %[[WHILE_BODY]]
+; CHECK: [[WHILE_END]]:
+; CHECK-NEXT: ret void
+;
+entry:
+ br label %while.body
+
+while.body:
+ %n_val = phi i32 [ %dec, %while.body ], [ %n, %entry ]
+ %src_val = phi ptr [ %src_val.next, %while.body ], [ %src, %entry ]
+ %dec = add nsw i32 %n_val, -1
+ %dst_val = load ptr, ptr %dst, align 8
+ %src_val.next = getelementptr inbounds nuw i8, ptr %src_val, i64 1
+ %dst_val.next = getelementptr inbounds nuw i8, ptr %dst_val, i64 1
+ store ptr %dst_val.next, ptr %dst, align 8
+ %val = load i8, ptr %src_val, align 1
+ store i8 %val, ptr %dst_val, align 1
+ %tobool.not = icmp eq i32 %dec, 0
+ br i1 %tobool.not, label %while.end, label %while.body
+
+while.end:
+ ret void
+}
+
+; A memcpy-like loop where both the source and destination pointers are loaded from pointers
+; FIXME: We could hoist the loads of both addresses, but doing the bounds check
+; of the store through the destination address itself requires using the hoisted
+; load.
+define void @memcpy_load_src_dst(ptr %dst, ptr %src, i32 %n) {
+; CHECK-LABEL: define void @memcpy_load_src_dst(
+; CHECK-SAME: ptr [[DST:%.*]], ptr [[SRC:%.*]], i32 [[N:%.*]]) {
+; CHECK-NEXT: [[ENTRY:.*]]:
+; CHECK-NEXT: br label %[[WHILE_BODY:.*]]
+; CHECK: [[WHILE_BODY]]:
+; CHECK-NEXT: [[N_VAL:%.*]] = phi i32 [ [[DEC:%.*]], %[[WHILE_BODY]] ], [ [[N]], %[[ENTRY]] ]
+; CHECK-NEXT: [[DEC]] = add nsw i32 [[N_VAL]], -1
+; CHECK-NEXT: [[SRC_VAL:%.*]] = load ptr, ptr [[SRC]], align 8
+; CHECK-NEXT: [[DST_VAL:%.*]] = load ptr, ptr [[DST]], align 8
+; CHECK-NEXT: [[SRC_VAL_NEXT:%.*]] = getelementptr inbounds nuw i8, ptr [[SRC_VAL]], i64 1
+; CHECK-NEXT: [[DST_VAL_NEXT:%.*]] = getelementptr inbounds nuw i8, ptr [[DST_VAL]], i64 1
+; CHECK-NEXT: store ptr [[SRC_VAL_NEXT]], ptr [[SRC]], align 8
+; CHECK-NEXT: store ptr [[DST_VAL_NEXT]], ptr [[DST]], align 8
+; CHECK-NEXT: [[VAL:%.*]] = load i8, ptr [[SRC_VAL]], align 1
+; CHECK-NEXT: store i8 [[VAL]], ptr [[DST_VAL]], align 1
+; CHECK-NEXT: [[TOBOOL_NOT:%.*]] = icmp eq i32 [[DEC]], 0
+; CHECK-NEXT: br i1 [[TOBOOL_NOT]], label %[[WHILE_END:.*]], label %[[WHILE_BODY]]
+; CHECK: [[WHILE_END]]:
+; CHECK-NEXT: ret void
+;
+entry:
+ br label %while.body
+
+while.body:
+ %n_val = phi i32 [ %dec, %while.body ], [ %n, %entry ]
+ %dec = add nsw i32 %n_val, -1
+ %src_val = load ptr, ptr %src, align 8
+ %dst_val = load ptr, ptr %dst, align 8
+ %src_val.next = getelementptr inbounds nuw i8, ptr %src_val, i64 1
+ %dst_val.next = getelementptr inbounds nuw i8, ptr %dst_val, i64 1
+ store ptr %src_val.next, ptr %src, align 8
+ store ptr %dst_val.next, ptr %dst, align 8
+ %val = load i8, ptr %src_val, align 1
+ store i8 %val, ptr %dst_val, align 1
+ %tobool.not = icmp eq i32 %dec, 0
+ br i1 %tobool.not, label %while.end, label %while.body
+
+while.end:
+ ret void
+}
|
|
✅ With the latest revision this PR passed the C/C++ code formatter. |
|
The "upcoming patch" I mention that will be adjusting LoopAccessAnalysis is john-brawn-arm@5609bbd |
|
Ping. |
fhahn
reviewed
Apr 22, 2025
There was a problem hiding this comment.
Would it help to use https://github.com/llvm/llvm-project/blob/main/llvm/include/llvm/Transforms/Utils/LoopVersioning.h#L100 here?
Yes, it looks like that would let us remove LoopVersioningLICM::setNoAliasToLoop. The alias metadata that's generated is slightly different, though it looks like it's more precise - rather than generating a single alias.scope that the loads/stores don't alias, we instead have two alias.scope which don't alias each other. |
|
Ping |
nikic
reviewed
Apr 30, 2025
| if (!any_of(Pointers, [&](auto &P) { return P.PointerValue == Ptr; })) { | ||
| LLVM_DEBUG(dbgs() << " Found a store without a runtime check.\n"); | ||
| return false; | ||
| } |
There was a problem hiding this comment.
Is this check needed for this patch, or for the future LAA change you have in mind?
There was a problem hiding this comment.
I think it makes sense to do this change in this patch, as it's part of handling the case where only some pointers have had checks generated for them.
There was a problem hiding this comment.
Hmm, but doesn't LVer.annotateLoopWithNoAlias() automatically take care of this? It should only add annotations to instructions it has RT checks for
There was a problem hiding this comment.
I believe this is not intended as a legality check (maybe it shouldn't be in instructionSafeForVersioning...) but as a profitability check, on the reasoning that missing noalias metadata on a store will prevent LICM of accesses.
But thinking about this more carefully, I'm not sure this heuristic really makes sense. A loop may well have perfectly analyzable stores that will not interfere with LICM of other accesses (say if the store is to a non-escaped alloca).
There was a problem hiding this comment.
Yes, this is a profitability check. An example of where we need this is the function gep_loaded_offset_with_store in the added load-from-unknown-address.ll test. Without this check we version the loop, but LICM can't do anything and we end up with two identical copies of the loop. We could possibly have a more precise check here, but I think that's something to look at in the future not in this patch.
nikic
approved these changes
May 7, 2025
|
LLVM Buildbot has detected a new failure on builder Full details are available at: https://lab.llvm.org/buildbot/#/builders/18/builds/15860 Here is the relevant piece of the build log for the reference |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Currently when we version a loop all loads and stores have the noalias metadata added to them. If there were some pointers that could not be analysed, and thus we could not generate runtime aliasing checks for, then we should not mark loads and stores using these pointers as noalias.
This is done by getting rid of setNoAliasToLoop and instead using annotateLoopWithNoAlias, as that already correctly handles partial alias information. This does result in slightly different aliasing metadata being generated, but it looks like it's more precise.
Currently this doesn't result in any change to the transforms that LoopVersioningLICM does, as LoopAccessAnalysis discards all results if it couldn't analyse every pointer leading to no loop versioning happening, but an upcoming patch will change that and we need this first otherwise we incorrectly mark some pointers as noalias even when they aren't.