[PATCH] D136525: [M68k] Add codegen pattern for atomic load / store

[Min-Yih Hsu via Phabricator via llvm-commits]

myhsu added a comment.

I think the patch is in shape now. I only have some minor comments. Thanks for the works :-)



================
Comment at: llvm/lib/Target/M68k/M68kISelLowering.cpp:169
+  setOperationAction(
+      {
+          ISD::ATOMIC_LOAD_ADD,
----------------
just want to double check, were these line formatted by clang-format?


================
Comment at: llvm/lib/Target/M68k/M68kInstrAtomics.td:41
+            (!cast<MxInst>("CAS"#size) !cast<MxRegOp>("MxDRD"#size):$cmp,
+                                        !cast<MxRegOp>("MxDRD"#size):$new,
+                                        !cast<MxMemOp>("MxARI"#size):$ptr)>;
----------------
nit: align with !cast in the previous line. ditto for the following line.


================
Comment at: llvm/lib/Target/M68k/M68kInstrAtomics.td:44
+}
+}
----------------
nit: "// let Predicates = [AtLeastM68020]"


================
Comment at: llvm/lib/Target/M68k/M68kInstrAtomics.td:10
+// FIXME: This is only supported on MC68020 and later.
+class MxCASOp<bits<2> size_encoding, MxType type>
+    : MxInst<(outs type.ROp:$out),
----------------
0x59616e wrote:
> myhsu wrote:
> > 0x59616e wrote:
> > > RKSimon wrote:
> > > > Wrap this inside FeatureISA20 ?
> > > Is there any example I can refer to ?
> > you can use `let Predicates = [IsM68020] in { ...` to wrap both the instruction definitions and the patterns (the predicates won't be honored if you only wrap the instruction definitions, since we're using custom patterns). Though I just realized that the `IsM680X0` predicates defined in M68kInstrInfo.td are wrong. I will fix that shortly.
> Something like D137425 ?
> Something like D137425 ?

Yes thank you!


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

https://reviews.llvm.org/D136525