[PATCH] D48128: [ARM] Parallel DSP IR Pass

[Sam Parker via Phabricator via llvm-commits]

samparker added inline comments.


================
Comment at: lib/Transforms/Scalar/ParallelDSP.cpp:142
+
+  if (match(V, m_ConstantInt(CInt))) {
+    // If a constant is used, it needs to fit within the bit width.
----------------
How are constants handled when generating the parallel instructions?


================
Comment at: lib/Transforms/Scalar/ParallelDSP.cpp:159
+  bool isNarrow = false;
+
+  if (match(V, m_Trunc(m_Value(Val)))) {
----------------
Would you mind commenting on what you're allowing here? I'm a bit confused as why you don't have to handle Mul nodes.


================
Comment at: lib/Transforms/Scalar/ParallelDSP.cpp:163
+      isNarrow = IsNarrowSequence<BitWidth>(Val, VL);
+  } else if (match(V, m_Add(m_Value(LHS), m_Value(RHS)))) {
+    if (IsNarrowSequence<BitWidth>(LHS, VL) &&
----------------
What happens for a series of adds that aren't going to be apart of the SMLAD?


================
Comment at: lib/Transforms/Scalar/ParallelDSP.cpp:321
+          LLVM_DEBUG(dbgs() << "OK: found two pairs of parallel loads!\n");
+          PMACPair P = std::make_pair(&PMul0, &PMul1);
+          R.PMACPairs.push_back(P);
----------------
So you know that the loads are sequential, but how do you know they can be loaded in parallel? I don't use any use of alias analysis.


================
Comment at: lib/Transforms/Scalar/ParallelDSP.cpp:364
+  for (PHINode &Phi : Header->phis()) {
+    if (!Phi.getType()->isIntegerTy())
+      continue;
----------------
It's probably worth checking that the integer is the type we support too so we can continue early and avoid the unnecessary, and more expensive, call below.


================
Comment at: lib/Transforms/Scalar/ParallelDSP.cpp:523
+  Value* Args[] = { VecLd0, VecLd1, Acc };
+  Function *SMLAD = Intrinsic::getDeclaration(M, Intrinsic::arm_smlad);
+
----------------
We also need to check that the DSP instructions are supported. There also should be a check, or an assert, that this pass is being run on an Arm subtarget.


https://reviews.llvm.org/D48128