[PATCH] D61020: [ARM][ParallelDSP] Relax alias checks

[Sjoerd Meijer via Phabricator via llvm-commits]

SjoerdMeijer accepted this revision.
SjoerdMeijer added a comment.
This revision is now accepted and ready to land.

This looks okay to me; just some nits inline.



================
Comment at: lib/Target/ARM/ARMParallelDSP.cpp:323
+
+  // Collect loads and instruction that may write to memory. For now we only
+  // record loads which are simple, sign-extended and have a single user.
----------------
nit: instruction -> instructions


================
Comment at: lib/Target/ARM/ARMParallelDSP.cpp:344
+    for (auto Write : Writes) {
+      if (Read == Write)
+        continue;
----------------
nit: I don't think this condition can be true. The loads/reads are Simple loads (not atomics), and the only writes that can be loads are loads with atomic ordering.


================
Comment at: lib/Target/ARM/ARMParallelDSP.cpp:368
+      for (auto Before : WritesBefore) {
+
+        // We can't move the second load backward, past a write, to merge
----------------
nit: newline


================
Comment at: lib/Target/ARM/ARMParallelDSP.cpp:618
+      const auto *Ty = Phi.getType();
+      if (!Ty->isIntegerTy(32) && !Ty->isIntegerTy(64))
+        continue;
----------------
Not even sure if it is possible, but looking at this condition I am wondering about i128s.




================
Comment at: lib/Target/ARM/ARMParallelDSP.cpp:675
+
+  assert((Base->hasOneUse() && Offset->hasOneUse() && BaseSExt && OffsetSExt)
+         && "Loads should have a single, extending, user");
----------------
Don't think we need to check for hasOneUse again here; we only add loads with one use to the list. Asserts for BaseSExt and OffetSext might be useful.


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

https://reviews.llvm.org/D61020