[PATCH] D126533: [LAA] Relax pointer dependency with runtime pointer checks

[Sander de Smalen via Phabricator via llvm-commits]

sdesmalen added a comment.

Hi @dtemirbulatov thanks for adding some more tests! There is still an unaddressed question  around the performance regression that would be good to get cleared up. I've also left a few comments on the new test.



================
Comment at: llvm/lib/Analysis/LoopAccessAnalysis.cpp:1704
+
+    bool SrcAffine = StrideAPtr;
+    if (!SrcAffine && !SrcInvariant && isa<SCEVAddRecExpr>(Src) &&
----------------
sdesmalen wrote:
> I think the expression below is equivalent, because if `Src` is not an affine `SCEVAddRecExpr`, then `StrideAPtr` must be zero?
> 
>   bool SrcAffine = !SrcInvariant && isa<SCEVAddRecExpr>(Src) && cast<SCEVAddRecExpr>(Src)->isAffine();
Unaddressed comment.


================
Comment at: llvm/lib/Analysis/LoopAccessAnalysis.cpp:1714-1715
+
+    if (APtr != BPtr && (SrcAffine || SinkAffine) &&
+        (SrcInvariant || SinkInvariant))
+      FoundNonConstantDistanceDependence = true;
----------------
dtemirbulatov wrote:
> dtemirbulatov wrote:
> > fhahn wrote:
> > > dtemirbulatov wrote:
> > > > sdesmalen wrote:
> > > > > This expression doesn't allow the case where `SrcAffine && SinkAffine && !SrcInvariant && !SinkInvariant`.
> > > > > 
> > > > > AIUI the distance can only be computed if:
> > > > > 
> > > > >   (SrcAffine && SinkAffine) ||
> > > > >   (SrcAffine && SinkInvariant) ||
> > > > >   (SrcInvariant && SinkAffine) ||
> > > > >   (SrcInvariant && SinkInvariant)
> > > > > 
> > > > > By the way, I couldn't see any new test-cases. Did you forget to add these, or did I miss something?
> > > > Hmm , For the case if both are invariants we don't need to emit a run-time check and I think we can ignore such cases. For both are affine type pointers I have not considered to support with the change and if I allow it I noticed performance regressions.
> > > It would be good to have at least all those combinations covered in the unit tests. Do you have any insight in the cause for the performance regression. you are seeing?
> > Added dedicated test.
> just extra check instructions with a small number of iterations.
> For both are affine type pointers I have not considered to support with the change and if I allow it I noticed performance regressions
Do you know why there are performance regressions? 



================
Comment at: llvm/test/Transforms/LoopVectorize/runtime-check-invariant-and-affine.ll:1
+; RUN: opt < %s -passes=loop-vectorize -force-vector-width=4 -S | FileCheck %s
+%struct.f = type { %struct.c, [0 x %struct.c] }
----------------
This should probably be a LoopAccessAnalysis test instead, so please use `print-access-info` instead of `loop-vectorize` and move this test to the corresponding directory.


================
Comment at: llvm/test/Transforms/LoopVectorize/runtime-check-invariant-and-affine.ll:6
+
+ at h = dso_local local_unnamed_addr global i32 0, align 4
+ at g = dso_local local_unnamed_addr global %struct.f zeroinitializer, align 2
----------------
nit: Can you please clean up things like `dso_local` and `local_unnamed_addr`, as they are not required for this test.


================
Comment at: llvm/test/Transforms/LoopVectorize/runtime-check-invariant-and-affine.ll:33
+  %indvars.iv = phi i64 [ %0, %for.body.preheader ], [ %indvars.iv.next, %for.body ]
+  %arrayidx = getelementptr inbounds [0 x %struct.c], ptr getelementptr inbounds (%struct.f, ptr @g, i64 1, i32 0, i32 0), i64 0, i64 %indvars.iv
+  %b = getelementptr inbounds [0 x %struct.c], ptr getelementptr inbounds (%struct.f, ptr @g, i64 1, i32 0, i32 0), i64 0, i64 %indvars.iv, i32 1
----------------
Did you get this output from Clang or did you modify it manually? I'm asking because the `inbounds` doesn't look right to me because it is accessing beyond the object `@g` at `@g + sizeof(%struct.f)`. Should this just be `ptr getelementptr inbounds (%struct.f, ptr @g, i32 0)` ?

Also, is the outer gep equivalent to:

  getelementptr inbounds %struct.c, (ptr ...), i64 %indvars.iv

?


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D126533/new/

https://reviews.llvm.org/D126533