[PATCH] D21571: [AArch64] Avoid generating indexed vector instructions for Exynos

[Renato Golin via llvm-commits]

rengolin added a comment.

> This optimization does not deserve, in my opinion, a new pass but it may be good idea to create a new pass so that we can include future similar optimizations in there. Let me know what you think. Thanks for looking at this.


I'm not a big fan of adding optimisations to AArch64InstrInfo because that's a place for more generic codegen. I agree with you, this is a bit heavy for such a small gain on a single core, but it's also a big piece of unrelated code to live inside InstrInfo.

It's possible that we could fuse all those passes into one, so we don't have to iterate over everything multiple times. But maybe this is not a job for this commit.

As it stands, I'm happy either way, as long as we make this more sensibly in due time. @t.p.northover, any comments on that?

However, my inline comments are still pertinent, no matter the choice, since that code will land on a new file or in AArch64InstrInfo anyway.

cheers,
--renato


================
Comment at: llvm/lib/Target/AArch64/AArch64VectorByElementOpt.cpp:83
@@ +82,3 @@
+/// Return true if replacement is recommended.
+bool AArch64VectorByElementOpt::replaceInstruction(MachineFunction *MF,
+       const MCInstrDesc *InstDesc, const MCInstrDesc *InstDescRep1,
----------------
Nit: "replaceInstruction" sounds like it actually replaces the instruction. This should be named "shouldReplaceInstruction".

================
Comment at: llvm/lib/Target/AArch64/AArch64VectorByElementOpt.cpp:257
@@ +256,3 @@
+    unsigned SrcReg2 = MI.getOperand(3).getReg();
+    bool Src2IsKill = MI.getOperand(3).isKill();
+    unsigned LaneNumber = MI.getOperand(4).getImm();
----------------
You only use Src2IsKill to set getKillRegState(Src2IsKill), maybe you should cache the result of getKillRegState(Src2IsKill) instead? Same to others.

================
Comment at: llvm/lib/Target/AArch64/AArch64VectorByElementOpt.cpp:263
@@ +262,3 @@
+    if (!reuseDUP(MI, DupMCID->getOpcode(), SrcReg2, LaneNumber, &DupDest)) {
+      DupDest = MRI.createVirtualRegister(RC);
+      BuildMI(MBB, MI, DL, *DupMCID, DupDest)
----------------
Why are you setting DupDest in reuseDUP, and re-setting it here?

Same for 4 ops below.

================
Comment at: llvm/lib/Target/AArch64/AArch64VectorByElementOpt.cpp:267
@@ +266,3 @@
+              .addImm(LaneNumber);
+    }
+    BuildMI(MBB, MI, DL, *MulMCID, MulDest)
----------------
Coding style: no newline between "}" and "else". All other cases, too.

================
Comment at: llvm/lib/Target/AArch64/AArch64VectorByElementOpt.cpp:284
@@ +283,3 @@
+            .addReg(DupDest, getKillRegState(Src1IsKill));
+  }
+  else
----------------
This looks odd and may be prone to future errors. Please change to:

    } else {
      return false;
    }

================
Comment at: llvm/lib/Target/AArch64/AArch64VectorByElementOpt.cpp:315
@@ +314,3 @@
+        // MI is deleted
+        LocalMIs.erase(&MI);
+        Changed = true;
----------------
This seems like an odd pattern. Can't you just add the MIs to a list when they change and delete all of them in the end?

================
Comment at: llvm/test/CodeGen/AArch64/arm64-neon-2velem.ll:387
@@ +386,3 @@
+; EXYNOS-LABEL: test_vfma_lane_f32:
+; EXYNOS: fmla {{v[0-9]+}}.2s, {{v[0-9]+}}.2s, {{v[0-9]+}}.2s
+; EXYNOS-NEXT: ret
----------------
What about the dups?


https://reviews.llvm.org/D21571