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

[Sam Parker via Phabricator via llvm-commits]

samparker marked 3 inline comments as done.
samparker added inline comments.


================
Comment at: lib/Target/ARM/ARMParallelDSP.cpp:344
+    for (auto Write : Writes) {
+      if (Read == Write)
+        continue;
----------------
SjoerdMeijer wrote:
> 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.
Makes sense, good catch!


================
Comment at: lib/Target/ARM/ARMParallelDSP.cpp:618
+      const auto *Ty = Phi.getType();
+      if (!Ty->isIntegerTy(32) && !Ty->isIntegerTy(64))
+        continue;
----------------
SjoerdMeijer wrote:
> Not even sure if it is possible, but looking at this condition I am wondering about i128s.
> 
> 
The IR supports arbitrary bit widths, but we only support 32- and 64-bit macs.


================
Comment at: lib/Target/ARM/ARMParallelDSP.cpp:675
+
+  assert((Base->hasOneUse() && Offset->hasOneUse() && BaseSExt && OffsetSExt)
+         && "Loads should have a single, extending, user");
----------------
SjoerdMeijer wrote:
> 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.
Cheers, I'll remove the check - and we are asserting on SExt!


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

https://reviews.llvm.org/D61020