[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 IMHO. I'm afraid I might not find time soon to look at all the details, but I've added a few more comments on style.

Thanks,

Kristof



================
Comment at: llvm/lib/Target/AArch64/AArch64VectorByElementOpt.cpp:107-108
+  /// latency of the original instruction to the latency of the replacement
+  /// instructions. We only check for a representative instruction in the class
+  /// instructions. We only check for a representative instruction in the class
+  /// of instructions and not all concerned instructions. For the VectorElem
----------------
duplicated line in comment.


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


================
Comment at: llvm/lib/Target/AArch64/AArch64VectorByElementOpt.cpp:229
+bool AArch64VectorByElementOpt::earlyExitVectElement(MachineFunction *MF,
+                                                     Subpass sp) {
   std::map<unsigned, bool> VecInstElemTable;
----------------
According to the coding standard, sp should probably be SP?


================
Comment at: llvm/lib/Target/AArch64/AArch64VectorByElementOpt.cpp:492-554
+  switch (MI.getOpcode()) {
+  default:
+    return false;
+  // ST2 case
+  case AArch64::ST2Twov2d:
+    prepareStmtParam(AArch64::ZIP1v2i64, AArch64::ZIP2v2i64, RC128, ReplInstrMCID,
+      ZipDest, TwoVectors);
----------------
I think this code might get easier to understand/maintain if the `prepareStmtParam` call happened after the switch, and in the switch, there would only be functionality where the different case statements actually differ. e.g. something like:


```
switch (...) {
  case A:
     Zip1 = Zip1v16i8; Zip2=ZIP2v16i8; RC=RC64; NumVec = FourVectors;
  case B:
     Zip1 = Zip1v8i8; Zip2=ZIP2v8i8; RC=RC64; NumVec = FourVectors;
  ...
}
prepareStmtParam(Zip1, Zip2, RC, NumVec)
```

Or maybe it would be even easier if this switch would be pulled apart into 4 switch statements, each setting one of Zip1/Zip2/RC/NumVec variables in the example above?
What do you think?


================
Comment at: llvm/lib/Target/AArch64/AArch64VectorByElementOpt.cpp:734-778
+  // A simple check to exit the by vector element sub-pass early for targets
+  // that do not need it.
+  if (!earlyExitVectElement(&MF, VectorElem))
+  {
+    std::map<unsigned, bool> VecInstElemTable;
+    SmallVector<MachineInstr *, 8> RemoveMIs;
+    for (MachineBasicBlock &MBB : MF) {
----------------
This looks like a lot of copy paste.
Couldn't this be rewritten to be a loop over the optimizers in this pass, something like (hand-wavy):
```
for (auto OptimizationKind : {VectorElem, Interleave}) {
...
}
```
?


https://reviews.llvm.org/D38196