[PATCH] D38196: [AArch64] Avoid interleaved SIMD store instructions for Exynos

[Kristof Beyls via Phabricator via llvm-commits]

kristof.beyls added a comment.

Hi Abderrazek,

Thanks for the new version, looks much better again.
I have made a few more point remarks.
But my main question at this point is: did you benchmark this? What are the results of that? On which cores/benchmarks?

Thanks!

Kristof



================
Comment at: llvm/lib/Target/AArch64/AArch64VectorByElementOpt.cpp:1
 //=- AArch64VectorByElementOpt.cpp - AArch64 vector by element inst opt pass =//
 //
----------------
I feel like the name of this pass/file should be changed, as the name no longer covers what it does.
It seems the pass now is becoming about replacing certain SIMD patterns with longer but more efficient SIMD patterns.
AArch64SIMDOptimizer might fit also with some of the file names of other optimizer passes already in the AArch64 target?


================
Comment at: llvm/lib/Target/AArch64/AArch64VectorByElementOpt.cpp:74
+    Interleave,
+    unsupportedSub
+  } Subpass;
----------------
s/unsupportedSub/UnsupportedSub/.
Do you need the UnsupportedSub value?


================
Comment at: llvm/lib/Target/AArch64/AArch64VectorByElementOpt.cpp:80
+    FourVectors,
+    unsupportedAcc
+  } AccessType;
----------------
Do you need the unsupportedAcc value?


================
Comment at: llvm/lib/Target/AArch64/AArch64VectorByElementOpt.cpp:166
+/// array InstDescRepl.
 /// Return true if replacement is recommended.
 bool AArch64VectorByElementOpt::shouldReplaceInstruction(
----------------
"Return true if replacement is expected to be faster" instead?
Also, I wouldn't duplicate the comments on both the function declarations and definitions - it's hard to keep them consistent.


================
Comment at: llvm/lib/Target/AArch64/AArch64VectorByElementOpt.cpp:171
     std::map<unsigned, bool> &VecInstElemTable) const {
-  // Check if replacment decision is alredy available in the cached table.
+  // Check if replacment decision is already available in the cached table.
   // if so, return it.
----------------
"replacement"


================
Comment at: llvm/lib/Target/AArch64/AArch64VectorByElementOpt.cpp:227
+/// Return true if early exit of the subpass is recommended.
+bool AArch64VectorByElementOpt::earlyExitVectElement(MachineFunction *MF,
+                                                     Subpass SP) {
----------------
It seems that "VectElement" isn't relevant in this function name anymore. Maybe it should become "shouldExitEarly" or something similar?


================
Comment at: llvm/lib/Target/AArch64/AArch64VectorByElementOpt.cpp:736-739
+        for (MachineBasicBlock::iterator MII = MBB.begin(), MIE = MBB.end();
+             MII != MIE;) {
+          MachineInstr &MI = *MII;
+          if (OptimizationKind == VectorElem)
----------------
bool InstRewrite can be declared inside this for loop; no need to declare it higher up.


https://reviews.llvm.org/D38196