[PATCH] D105612: [PowerPC] Implement quadword atomic load/store
Jinsong Ji via Phabricator via llvm-commits
llvm-commits at lists.llvm.org
Tue Aug 31 11:34:08 PDT 2021
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
More information about the llvm-commits
mailing list