[PATCH] D87879: [LoopInterchange] Add dominance check to guarantee output dependency order

Mon Nov 9 12:35:44 PST 2020

fhahn added a comment.

Thanks for updating the patch! Some small remaining suggestions inline.

================
Comment at: llvm/lib/Transforms/Scalar/LoopInterchange.cpp:152
+            // If the execution of either Src or Dst depends on IV-related
+            // condition, interchange may mess up memory write order and result
+            // in incorrect result.
----------------
nit: `conditions`?

I think it would also good to mention why we currently are not actually checking if the condition is IV-dependent. Presumably because it's expensive to find?

================
Comment at: llvm/lib/Transforms/Scalar/LoopInterchange.cpp:154
+            // in incorrect result.
+            // FIXME: If there're no loads access the same memory location of
+            // two ouptut dependent stores, it could be safe to interchange.
----------------
nit: `If there are no loads accessing the same memory location as the two output dependent stores....`.

================
Comment at: llvm/lib/Transforms/Scalar/LoopInterchange.cpp:157
+            if (D->isOutput() && !DT->dominates(Src, Dst))
+              Direction = '*';
+            else
----------------
it might be worth adding a LLVM_DEBUG message here, because it might be surprising to have the change here.

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

https://reviews.llvm.org/D87879