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

[Kristof Beyls via Phabricator via llvm-commits]

kristof.beyls added inline comments.


================
Comment at: llvm/lib/Target/AArch64/AArch64VectorByElementOpt.cpp:163-170
+bool AArch64SIMDInstrOpt::
+shouldReplaceInst(MachineFunction *MF, const MCInstrDesc *InstDesc,
+                  SmallVectorImpl<const MCInstrDesc*> &InstDescRepl) {
+  // Check if replacement decision is already available in the cached table.
   // if so, return it.
-  if (!VecInstElemTable.empty() &&
-      VecInstElemTable.find(InstDesc->getOpcode()) != VecInstElemTable.end())
-    return VecInstElemTable[InstDesc->getOpcode()];
+  if (!SIMDInstrTable.empty() &&
+      SIMDInstrTable.find(InstDesc->getOpcode()) != SIMDInstrTable.end())
----------------
Is the only reason you need to drop const from shouldReplaceInst and other methods that you're using operator[] here to get to an element that's guaranteed to be in the map?
If so, wouldn't it be better to use syntax "*SIMDInstrTable.find(InstDesc->getOpcode())", so that the const can remain on all those methods?


================
Comment at: llvm/lib/Target/AArch64/AArch64VectorByElementOpt.cpp:333-353
+      return false;
+    ReplInstrMCID.clear();
+    OriginalMCID = &TII->get(AArch64::ST4Fourv16b);
+    for (unsigned i=0; i<4; i++) {
+      ReplInstrMCID.push_back(&TII->get(AArch64::ZIP1v16i8));
+      ReplInstrMCID.push_back(&TII->get(AArch64::ZIP2v16i8));
+    }
----------------
>From a logical point of view, this is a lot of code that repeats information encoded in a different way in the big switch statements in optimizeLdStInterleave.
It'll be easy to update one location in the future and forget the other location.
Couldn't the logic in the switch statements in optimizeLdStInterleave be reused here somehow, so that the information is only encoded once?


https://reviews.llvm.org/D38196