[PATCH] D88389: [M68k] (Patch 3/8) Basic infrastructures and target description files

Sun Dec 20 10:46:08 PST 2020

jrtc27 added inline comments.

================
Comment at: llvm/cmake/modules/HandleLLVMOptions.cmake:333
+# (i.e. Use 16-bit align and fix LLVM) is non-trivial. This means LLVM can
+# not run natively on M68k CPU that doesn't have 32-bit bus.
+if(LLVM_INFERRED_HOST_TRIPLE STREQUAL "m68k-unknown-linux-gnu")
----------------
That is not true. The comment in the GCC manpage is just that if you have a 32-bit data bus then it's faster to align integers (as otherwise you might need to do *two* bus transactions if the data is only 2-byte aligned). But if you have a 16-bit bus both are equally slow and work (and you end up wasting a bit more memory for padding without any benefit).

================
Comment at: llvm/cmake/modules/HandleLLVMOptions.cmake:326-330
+# GCC m68k on Linux by default aligns on 16bit, we want 32
+if(LLVM_INFERRED_HOST_TRIPLE STREQUAL "m68k-unknown-linux-gnu")
+    set(CMAKE_CXX_FLAGS "${CMAKE_CXX_FLAGS} -malign-int")
+    set(CMAKE_C_FLAGS "${CMAKE_C_FLAGS} -malign-int")
+endif()
----------------
RKSimon wrote:
> jrtc27 wrote:
> > This is a non-standard ABI and thus is incompatible with system libraries, so IMO is a blocker to merging a native m68k LLVM build. You can still merge cross-compilation support without this though.
> I don't think this is a blocker but you should more explicitly mention the potential issue on native in the comment.
I do not want this to land. It is broken and should never be used unless you are very very careful. If you want to compile LLVM and take the risk of ABI issues, you can manually add -malign-int to CMAKE_C[XX]_FLAGS when invoking CMake yourself.

================
Comment at: llvm/lib/Target/M68k/M68k.td:21-22
+
+def FeatureISAx00
+  : SubtargetFeature<"x00", "SubtargetKind", "M00", "Is M68000 ISA supported">;
+
----------------
These are really weird feature names. 680x0 -> 68x00, 68x10, etc? What's with the "M" prefix on the two digits? Just call them FeatureISA68000 etc? Also isn't 68000 support the baseline and thus not in need of a subtarget feature?

================
Comment at: llvm/lib/Target/M68k/M68kCallingConv.td:31
+/// M68k C return convention.
+def RetCC_M68k_C : CallingConv<[
+  CCIfType<[i1],   CCPromoteToType<i8>>,
----------------
How do you deal with the differing return conventions for integers and pointers (data and argument registers respectively)?

================
Comment at: llvm/lib/Target/M68k/M68kCallingConv.td:87
+
+def CC_M68k_C : CallingConv<[
+  /// Promote i1/i8/i16 arguments to i32.
----------------
How do you handle the `sret` argument? That needs to go in %a0, and also be implicitly returned in %a0.

================
Comment at: llvm/lib/Target/M68k/M68kCallingConv.td:12
+/// conventions assume Int to be 4 bytes and 4 byte aligned. Short variant is
+/// not supported yet.
+///
----------------
RKSimon wrote:
> /// TODO Short variant is not supported yet.
I don't see any merit in supporting an ABI in LLVM where int is only 2 bytes, so would just drop this TODO entirely. Especially since current POSIX requires int to be at least 32-bit.

================
Comment at: llvm/lib/Target/M68k/M68kInstrArithmetic.td:173
+
+// op $mem, $reg
+def NAME#"8dk"  : MxBiArOp_RFRM<MN, NODE, MxType8d,  MxType8.KOp,  MxType8.KPat,  CMD, MxEncEAk, MxExtBrief_2>;
----------------
Indent.

================
Comment at: llvm/lib/Target/M68k/M68kInstrArithmetic.td:240
+let Pattern = [(null_frag)] in
+multiclass MxBiArOp_AF<string MN, SDNode NODE, bit isComm, bits<4> CMD, bits<4> CMDI> {
+
----------------
Indent.

================
Comment at: llvm/lib/Target/M68k/M68kInstrArithmetic.td:279
+
+let isCommutable = isComm in {
+
----------------
Indent.

================
Comment at: llvm/lib/Target/M68k/M68kInstrArithmetic.td:303
+
+let isCommutable = 1 in {
+
----------------
Indent.

================
Comment at: llvm/lib/Target/M68k/M68kInstrArithmetic.td:371
+multiclass MMxCmp_RM<MxType TYPE> {
+def NAME#TYPE.KOp.Letter : MxCmp_RM<TYPE, TYPE.KOp, TYPE.KPat, MxEncEAk,   MxExtBrief_1>;
+def NAME#TYPE.QOp.Letter : MxCmp_RM<TYPE, TYPE.QOp, TYPE.QPat, MxEncEAq,   MxExtI16_1>;
----------------
Indent.

================
Comment at: llvm/lib/Target/M68k/M68kInstrArithmetic.td:379
+multiclass MMxCmp_MI<MxType TYPE> {
+def NAME#TYPE.KOp.Letter#"i" : MxCmp_MI<TYPE, TYPE.KOp, TYPE.KPat, MxEncEAk,   MxExtBrief_1>;
+def NAME#TYPE.QOp.Letter#"i" : MxCmp_MI<TYPE, TYPE.QOp, TYPE.QPat, MxEncEAq,   MxExtI16_1>;
----------------
Indent.

================
Comment at: llvm/lib/Target/M68k/M68kInstrArithmetic.td:387
+foreach S = [8, 16, 32] in {
+def CMP#S#dd : MxCmp_RR<!cast<MxType>("MxType"#S#"d")>;
+def CMP#S#di : MxCmp_RI<!cast<MxType>("MxType"#S#"d")>;
----------------
Indent.

================
Comment at: llvm/lib/Target/M68k/M68kInstrArithmetic.td:472
+
+let isCommutable = isComm in {
+def "S"#NAME#"d32d16" : MxDiMuOp_DD<MN#"s", CMD, MxSDiMuOpmode, MxDRD32, MxDRD16>;
----------------
Indent.

================
Comment at: llvm/lib/Target/M68k/M68kInstrArithmetic.td:662
+
+// add reg, reg
+def : Pat<(!cast<SDNode>(node) i8 :$src, i8 :$opd), (ADD8dd  MxDRD8 :$src, MxDRD8 :$opd)>;
----------------
Indent.

================
Comment at: llvm/lib/Target/M68k/M68kInstrArithmetic.td:711
+// These patterns treat AL value as immediate
+/* def : Pat<(!cast<SDNode>(node) MxType32r.ROp:$src, MxType32r.BPat:$opd), */
+/*           (ADD32ri MxXRD32:$src, MxType32r.IOp:$opd)>; */
----------------
Why are they commented out? If you need them, uncomment them, if you don't, remove them.

================
Comment at: llvm/lib/Target/M68k/M68kInstrArithmetic.td:733
+
+// sub reg, reg
+def : Pat<(!cast<SDNode>(node) i8 :$src, i8 :$opd), (SUB8dd  MxDRD8 :$src, MxDRD8 :$opd)>;
----------------
Indent.

================
Comment at: llvm/lib/Target/M68k/M68kInstrArithmetic.td:789
+// and reg, mem
+/* def : Pat<(and MxDRD8:$src, (Mxloadi8 addr:$opd)), */
+/*           (AND8rm MxDRD8:$src, addr:$opd)>; */
----------------
Ditto re commenting.

================
Comment at: llvm/lib/Target/M68k/M68kInstrArithmetic.td:810
+// xor reg, mem
+/* def : Pat<(xor MxDRD8:$src, (Mxloadi8 addr:$opd)), */
+/*           (XOR8rm MxDRD8:$src, addr:$opd)>; */
----------------
Ditto.

================
Comment at: llvm/lib/Target/M68k/M68kInstrArithmetic.td:831
+// or reg/mem
+/* def : Pat<(or MxDRD8:$src, (Mxloadi8 addr:$opd)), */
+/*           (OR8rm MxDRD8:$src, addr:$opd)>; */
----------------
Ditto.

================
Comment at: llvm/lib/Target/M68k/M68kInstrCompiler.td:78
+
+def ADJCALLSTACKDOWN
+  : MxPseudo<(outs), (ins i32imm:$amt1, i32imm:$amt2), "#ADJCALLSTACKDOWN",
----------------
Indent.

================
Comment at: llvm/lib/Target/M68k/M68kInstrControl.td:131
+// Currently M68k does not allow 16 bit indirect jumps use sext operands
+/* def JMP16r     : MxInst<(outs), (ins M68k_ARI16:$dst), */
+/*                             "jmp\t$dst", */
----------------
This one makes sense to keep commented out given the FIXME, but please use // comments  (and using `/* .. */` multiple times, one per line, is especially bad).

================
Comment at: llvm/lib/Target/M68k/M68kInstrControl.td:154
+               "cs", "pl", "gt", "hi", "vc", "le", "vs"] in {
+def B#cc#"8"
+  : MxBcc<cc, MxBrTarget8, MxType8,
----------------
Indent.

================
Comment at: llvm/lib/Target/M68k/M68kInstrControl.td:217
+
+def : Pat<(MxCall (i32 tglobaladdr:$dst)),  (CALLq tglobaladdr:$dst)>,  Requires<[IsPIC]>;
+def : Pat<(MxCall (i32 texternalsym:$dst)), (CALLq texternalsym:$dst)>, Requires<[IsPIC]>;
----------------
```let Predicates = [IsPIC] in {```
and avoid duplication

================
Comment at: llvm/lib/Target/M68k/M68kInstrControl.td:244
+def TAILJMPj  : MxPseudo<(outs), (ins MxARI32_TC:$dst)>;
+}
+} // let Uses = [SP]
----------------

================
Comment at: llvm/lib/Target/M68k/M68kInstrControl.td:261
+                   [(MxRet timm:$adj)]>;
+} // let isCodeGenOnly = 1
+
----------------
No need for { }

================
Comment at: llvm/lib/Target/M68k/M68kInstrControl.td:263
+
+}
+
----------------
Comment

================
Comment at: llvm/lib/Target/M68k/M68kInstrControl.td:281
+                 [(set MxXRD32:$dst, (MxSetCC_C MxCONDcs, CCR))]>;
+} // isCodeGenOnly
+
----------------
Wrong comment

================
Comment at: llvm/lib/Target/M68k/M68kInstrData.td:95
+
+// NOTE tablegen defines implicitly created defs inside multiclass which
+// breaks some code like letting Pattern afterwards in _TC
----------------
Indent

================
Comment at: llvm/lib/Target/M68k/M68kInstrData.td:96
+// NOTE tablegen defines implicitly created defs inside multiclass which
+// breaks some code like letting Pattern afterwards in _TC
+
----------------
I don't understand this comment.

================
Comment at: llvm/lib/Target/M68k/M68kInstrData.td:146
+
+// MEM <- (An)+
+def NAME#TYPE.OOp.Letter#TYPE.Postfix
----------------
Indent.

================
Comment at: llvm/lib/Target/M68k/M68kInstrFormats.td:304
+
+/* class MxExtBrief<bits<3> opN, bits<1> wl = 0b1> */
+/*     : MxEncExt<MxBead8Imm<opN>,   MxBead1Bit<0b0>, */
----------------
Ditto re comments

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

https://reviews.llvm.org/D88389