[PATCH] D126533: [LAA] Relax pointer dependency with runtime pointer checks
Florian Hahn via Phabricator via llvm-commits
llvm-commits at lists.llvm.org
Fri Sep 9 00:23:42 PDT 2022
fhahn added inline comments.
================
Comment at: llvm/lib/Analysis/LoopAccessAnalysis.cpp:1827
+ if (!StrideAPtr || !StrideBPtr || StrideAPtr != StrideBPtr) {
+ bool SrcInvariant = PSE.getSE()->isLoopInvariant(Src, InnermostLoop);
+ bool SinkInvariant = PSE.getSE()->isLoopInvariant(Sink, InnermostLoop);
----------------
On `main`, there's now a `SE` variable that can be used instead of `PSE.getSE()`.
================
Comment at: llvm/lib/Analysis/LoopAccessAnalysis.cpp:1838
+
+ if (APtr != BPtr && (SrcAffine || SinkAffine))
+ FoundNonConstantDistanceDependence = true;
----------------
Could you add a comment here describing the logic here for future readers? It might be good to mention how the `A[B[I]` case is still avoided (we won't generate runtime checks because we won't be able to identify the bounds for the check).
================
Comment at: llvm/test/Analysis/LoopAccessAnalysis/runtime-check-invariant-and-affine.ll:17
+
+define void @two_sides_affine() {
+; CHECK-LABEL: function 'two_sides_affine'
----------------
I might have missed it but could you also add a test case where one side is affine and the other is neither strided nor invariant?
================
Comment at: llvm/test/Analysis/LoopAccessAnalysis/runtime-check-invariant-and-affine.ll:19
+; CHECK-LABEL: function 'two_sides_affine'
+; CHECK:Memory dependences are safe with run-time checks
+;
----------------
please also check the runtime checks to make sure the correct tones are generated.
================
Comment at: llvm/test/Analysis/LoopAccessAnalysis/runtime-check-invariant-and-affine.ll:38
+ %indvars.iv.next = add nsw i64 %indvars.iv, 1
+ %2 = and i64 %indvars.iv.next, 4294967295
+ %tobool.not = icmp eq i64 %2, 0
----------------
Is the more complicated condition needed here?
================
Comment at: llvm/test/Analysis/LoopAccessAnalysis/runtime-check-invariant-and-affine.ll:43
+for.cond.for.end_crit_edge:
+ store i32 0, ptr @h, align 4
+ br label %for.end
----------------
is this needed?
================
Comment at: llvm/test/Analysis/LoopAccessAnalysis/runtime-check-invariant-and-affine.ll:81
+for.cond.for.end_crit_edge:
+ store i32 0, i32* @a, align 4
+ br label %for.end
----------------
is this needed?
================
Comment at: llvm/test/Analysis/LoopAccessAnalysis/runtime-check-invariant-and-affine.ll:93
+entry:
+ %0 = load i32, i32* @n, align 4
+ %cmp10 = icmp sgt i32 %0, 0
----------------
pass this as i64 argument to avoid the unnecessary load/zext later? (same above)
================
Comment at: llvm/test/Analysis/LoopAccessAnalysis/runtime-check-invariant-and-affine.ll:95
+ %cmp10 = icmp sgt i32 %0, 0
+ br i1 %cmp10, label %for.body.preheader, label %for.cond.cleanup
+
----------------
the check and conditional branch shouldn't be needed.
================
Comment at: llvm/test/Analysis/LoopAccessAnalysis/runtime-check-invariant-and-affine.ll:108
+ %arrayidx = getelementptr %struct.Arrays, %struct.Arrays* @s1, i64 0, i32 1, i64 %indvars.iv
+ %2 = load double, double* %arrayidx, align 8
+ %arrayidx2 = getelementptr %struct.Arrays, %struct.Arrays* @s1, i64 0, i32 2, i64 %indvars.iv
----------------
typed pointers are deprecated, could you update the test to use opaque pointers instead here?
================
Comment at: llvm/test/Transforms/LoopDistribute/scev-inserted-runtime-check.ll:9
define void @f(i32* noalias %a, i32* noalias %b, i32* noalias %c, i32* noalias %d, i32* noalias %e, i64 %N) {
; CHECK-LABEL: @f(
----------------
This test doesn't seem to be testing what it is supposed to any longer. I am not sure what changed and the new behavior isn't incorrect - we just don't version, but it would be good to know why and restore the original behavior for the test.
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D126533/new/
https://reviews.llvm.org/D126533
More information about the llvm-commits
mailing list