[PATCH] D105612: [PowerPC] Implement quadword atomic load/store

[Jinsong Ji via Phabricator via llvm-commits]

jsji accepted this revision as: jsji.
jsji added a comment.
This revision is now accepted and ready to land.

LGTM with some nits. Thanks!



================
Comment at: llvm/lib/Target/PowerPC/PPCISelLowering.cpp:10447
+    if (Ty == MVT::i128)
+      // FIXME: Testing one of two paired registers is sufficient to guarantee
+      // ordering?
----------------
nit: multiline after `if` should be within `{`/`}`?


================
Comment at: llvm/lib/Target/PowerPC/PPCISelLowering.cpp:10526
+  switch (Opc) {
+  case ISD::ATOMIC_LOAD: {
+    SDVTList Tys = DAG.getVTList(MVT::i64, MVT::i64, MVT::Other);
----------------
Add comments to all the following code please.


================
Comment at: llvm/lib/Target/PowerPC/PPCISelLowering.cpp:12720
+  } else if (MI.getOpcode() == PPC::STQX_PSEUDO) {
+    DebugLoc DL = MI.getDebugLoc();
+    Register Ptr = MI.getOperand(0).getReg();
----------------
Looks like we can common up most of these code for LQX_PSEUDO/STQX_PSEUDO? Just need additional check for last BuildMI?


================
Comment at: llvm/lib/Target/PowerPC/PPCISelLowering.cpp:17363
   if (MemVT.isScalarInteger()) {
-    assert(Size <= 64 && "Not expecting scalar integers larger than 8 bytes!");
+    assert(Size <= 128 &&
+           "Not expecting scalar integers larger than 16 bytes!");
----------------
So we will set Flag to `PPC::MOF_DoubleWordInt` for 128.  Although we haven't used the flags for now, I think it would be better if we add one more flag for 128?


================
Comment at: llvm/lib/Target/PowerPC/PPCInstrInfo.cpp:3106
 
+    // FIXME: Maybe we can expand it in 'PowerPC Expand Atomic' pass.
   case PPC::CFENCE8: {
----------------
nit: indent is off?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D105612