[PATCH] D90290: [LoopInterchange] Prevent Loop Interchange for non-affine value store to affine access

[Florian Hahn via Phabricator via llvm-commits]

fhahn added a comment.

Thanks for the patch. Some initial comments inline.



================
Comment at: llvm/lib/Transforms/Scalar/LoopInterchange.cpp:952
+// If a value is loaded from non-affine access and
+// did finite number of calculation, we call it non-affine value.
+// If a value is loaded from affine access and
----------------
nit: re-flow comments


================
Comment at: llvm/lib/Transforms/Scalar/LoopInterchange.cpp:961
+    return false;
+  // check if Op is the indction variable of any loop contained in OutLoop
+  else {
----------------
Nit: start comment with a capital letter, end with period (`.`).


================
Comment at: llvm/lib/Transforms/Scalar/LoopInterchange.cpp:962
+  // check if Op is the indction variable of any loop contained in OutLoop
+  else {
+    auto *PHIOut = OutLoop->getInductionVariable(*SE);
----------------
Usually early returns are used to limit the indentation level. Could this just be something like

```
 if (!isa<Instruction>(*Op) || OutLoop->isLoopInvariant(Op))
   return false;

```
and drop the `else {` block?


================
Comment at: llvm/lib/Transforms/Scalar/LoopInterchange.cpp:984
+        dyn_cast<SCEVAddRecExpr>(SE->getSCEV(LdAddr));
+    if ((!LdAddrAddRec || !LdAddrAddRec->isAffine()) &&
+        !OutLoop->isLoopInvariant(Op))
----------------
just return the expression? It would also be good to add a brief comment explaining the condition.


================
Comment at: llvm/lib/Transforms/Scalar/LoopInterchange.cpp:992
+    auto *I = cast<Instruction>(Op);
+    for (Use &U : I->operands()) {
+      Res |= checkNonAffineValue(U.get(), SE, OutLoop, InnerLoop);
----------------
could this just be something like `return any_of(I->operands(), [](Use &U) {});`?


================
Comment at: llvm/test/Transforms/LoopInterchange/non-affine-store-to-affine.ll:1
+; RUN: opt < %s -basicaa -loop-interchange -S -debug 2>&1 | FileCheck %s
+
----------------
you need to add `; REQUIRES: asserts` when using `-debug`, so the test is not run if assertions. are disabled.


================
Comment at: llvm/test/Transforms/LoopInterchange/non-affine-store-to-affine.ll:41
+
+for.body4.i:                                      ; preds = %for.body4.i, %for.cond1.preheader.i
+  %indvars.iv.i = phi i64 [ 0, %for.cond1.preheader.i ], [ %indvars.iv.next.i, %for.body4.i ]
----------------
A few small  suggestions for the test case.

it would be helpful to update the names of the BBs to include more information, e.g. `outer.header`, `inner.body` or something like that. And re-order them in the file so they are in a more natural order like preheader, outer header, inner, outer.latch, exit.

Might also be good to have an exit block for the loop nest that returns and maybe clean up the names of the values (e.g. use `%j` for `%indvars.iv.i` `%j.next` for the step).


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

https://reviews.llvm.org/D90290