[PATCH] D70072: [ARM] Improve codegen of volatile load/store of i64

[Eli Friedman via Phabricator via llvm-commits]

efriedma added inline comments.


================
Comment at: llvm/lib/Target/ARM/ARMISelDAGToDAG.cpp:3487
+  }
+  case ARMISD::STRD: {
+    ARMStoreDualSDNode *ST = cast<ARMStoreDualSDNode>(N);
----------------
You should be able to use TableGen patterns for these, I think.  AArch64Prefetch is an example of something similar to what you want.


================
Comment at: llvm/lib/Target/ARM/ARMISelLowering.cpp:8969
+      !Subtarget->isThumb1Only() &&
+      LD->getExtensionType() == ISD::NON_EXTLOAD && LD->isVolatile()) {
+    SDLoc dl(N);
----------------
efriedma wrote:
> I'd prefer not to exclude extending loads here.  Could lead to weird cases where we miss the transform.
You still need to *handle* extending loads somehow...

Actually, maybe we don't mess with volatile loads in DAGCombine, and you don't need to implement it.  In that case, it would still be nice to have an assertion, in case someone changes it at some point.


================
Comment at: llvm/lib/Target/ARM/ARMISelLowering.cpp:9021
+  if (MemVT == MVT::i64 && Subtarget->hasV5TEOps() &&
+      !Subtarget->isThumb1Only() && !ST->isTruncatingStore() &&
+      ST->isVolatile()) {
----------------
We also don't want to restrict truncating stores.


================
Comment at: llvm/lib/Target/ARM/ARMISelLowering.cpp:9033
+        DAG.getVTList(MVT::Other),
+        {ST->getChain(), Lo, Hi, ST->getBasePtr(), ST->getOffset()}, dl, MemVT,
+        ST->getMemOperand());
----------------
Loads and stores should not have an "Offset" at this point; we don't form pre/post-indexed operations until after legalization.  Maybe worth asserting "isUnindexed()".  (Same for loads.)


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D70072