[PATCH] D119720: [ARM] Pass for Cortex-A57 and Cortex-A72 Fused AES Erratum

[Dave Green via Phabricator via cfe-commits]

dmgreen added a comment.

Looks OK to me, as far as I can see. If it worth adding a few extra instructions that may come up?



================
Comment at: llvm/lib/Target/ARM/ARMFixCortexA57AES1742098Pass.cpp:146
+  case ARM::VMVNd:
+  case ARM::VMVNq:
+  // VMOV of 64-bit value between D registers (when condition = al)
----------------
Perhaps add these, if they are safe:
VBICd/q
VBICi's, VORRi's
VBIF/VBIT/VBSL/VBSP
VCEQ/VCNE/etc?
VDUP? VEXT?
VMVN imm equivalents of VMOV's
VREV's?
VSHL's, VSHR's?
I'm not sure if they will be very useful, but they are the kind of instructions that may come up in aes algorithms.


================
Comment at: llvm/lib/Target/ARM/ARMTargetMachine.cpp:588-589
   addPass(createARMBranchTargetsPass());
+  // Inserts Constant Islands. No new instructions may be inserted after this
+  // point, as this will affect the offsets used for accessing these constants.
   addPass(createARMConstantIslandPass());
----------------
"No new instructions may be inserted" -> "Block sizes cannot be increased"
And maybe "will affect the offsets used for accessing these constants." -> "may push the branch ranges and load offsets of accessing constant pools out of range."


================
Comment at: llvm/lib/Target/ARM/ARMTargetMachine.cpp:591-593
+  // Finalises Low-Overhead Loops. This relies on knowing the final block size,
+  // but can run after constant islands as it does not insert additional
+  // instructions.
----------------
It's not about "not inserting instructions" exactly - it will replace psuedos with all kinds of new instructions :)
The pseudos needed to have a conservative size through ConstantIslandPass though to allow that though. It does make sure that it will not move instructions further apart from their targets.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D119720/new/

https://reviews.llvm.org/D119720