[PATCH] D88389: [M68k] (Patch 3/8) Basic infrastructures and target description files
Jessica Clarke via Phabricator via llvm-commits
llvm-commits at lists.llvm.org
Tue Feb 23 05:32:46 PST 2021
jrtc27 added inline comments.
================
Comment at: llvm/lib/Target/M68k/M68k.td:21-22
+
+def FeatureISAx00
+ : SubtargetFeature<"x00", "SubtargetKind", "M00", "Is M68000 ISA supported">;
+
----------------
RKSimon wrote:
> jrtc27 wrote:
> > 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?
> If the plan is to add support M68k-cousins (ColdFire etc.) in the future then we'll definitely need FeatureISAx00 as the "baseline" is below 68000. I don't have a problem keeping this tbh.
Huh, I didn't realise ColdFire lacked some of the base 68000 instructions, interesting.
================
Comment at: llvm/lib/Target/M68k/M68kCallingConv.td:18
+class CCIfSubtarget<string F, CCAction A>
+ : CCIf<!strconcat("static_cast<const M68kSubtarget&>"
+ "(State.getMachineFunction().getSubtarget()).", F), A>;
----------------
(yes, there are places in LLVM that get this wrong if you go looking)
================
Comment at: llvm/lib/Target/M68k/M68kCallingConv.td:40
+/// M68k fastcc return convention.
+/// This convention allows to return upto 16 bytes in registers which can be
+/// split among 16 1-byte values or used for a single 16-byte value.
----------------
================
Comment at: llvm/lib/Target/M68k/M68kInstrArithmetic.td:1
+//===-- M68kInstrArithmetic.td - Integer Arith Instrs ----*- tablegen -*-===//
+//
----------------
I think is needed to line it up, though check for yourself, it's hard to tell in this view
================
Comment at: llvm/lib/Target/M68k/M68kInstrArithmetic.td:43
+ MxEncEA EA, MxEncExt EXT>
+ : MxEncoding<EA.Reg, EA.DA, EA.Mode, OPMODE.B0, OPMODE.B1, OPMODE.B2, REG, CMD,
+ EXT.Imm, EXT.B8, EXT.Scale, EXT.WL, EXT.DAReg>;
----------------
Line needs wrapping one argument earlier
================
Comment at: llvm/lib/Target/M68k/M68kInstrArithmetic.td:44
+ : MxEncoding<EA.Reg, EA.DA, EA.Mode, OPMODE.B0, OPMODE.B1, OPMODE.B2, REG, CMD,
+ EXT.Imm, EXT.B8, EXT.Scale, EXT.WL, EXT.DAReg>;
+
----------------
Should be indented, if not lined up with the other arguments (preferable, but there are tons of examples in the tree where it's only indented, and you have a bunch of those too so I don't think it's a hard requirement to change all those, so long as they're at least indented to clearly show they're a continuation).
================
Comment at: llvm/lib/Target/M68k/M68kInstrArithmetic.td:75
+ : MxEncoding<DST_EA.Reg, DST_EA.DA, DST_EA.Mode, SIZE, CMD, MxBead4Bits<0>,
+ SRC_EXT.Imm, SRC_EXT.B8, SRC_EXT.Scale, SRC_EXT.WL, SRC_EXT.DAReg,
+ DST_EXT.Imm, DST_EXT.B8, DST_EXT.Scale, DST_EXT.WL, DST_EXT.DAReg>;
----------------
Indent or line up as above
================
Comment at: llvm/lib/Target/M68k/M68kInstrArithmetic.td:93
+ !cast<MxEncOpMode>("MxOpMode"#TYPE.Size#TYPE.RLet#"EA"),
+ MxBeadReg<0>, !cast<MxEncEA>("MxEncEA"#TYPE.RLet#"_2"), MxExtEmpty>>;
+
----------------
MxExtEmpty should be on its own line, and probably the !cast expression too as that'll make it fit in the (soft for TableGen) line limit (the line above would be more ugly broken up than as is)
================
Comment at: llvm/lib/Target/M68k/M68kInstrArithmetic.td:151
+ MN#"."#TYPE.Prefix#"\t$opd, $dst",
+ /* [(store (NODE (TYPE.Load MEMPat:$dst), TYPE.VT:$opd), MEMPat:$dst)], */
+ [],
----------------
Commented-out code
================
Comment at: llvm/lib/Target/M68k/M68kInstrArithmetic.td:153
+ [],
+ MxArithEncoding<MxBead4Bits<CMD>,
+ !cast<MxEncOpMode>("MxOpMode"#TYPE.Size#"EA"#TYPE.RLet),
----------------
Overindented
================
Comment at: llvm/lib/Target/M68k/M68kInstrArithmetic.td:162
+ MN#"."#TYPE.Prefix#"\t$opd, $dst",
+ /* [(store (NODE (TYPE.Load MEMPat:$dst), TYPE.IPat:$opd), MEMPat:$dst)], */
+ [],
----------------
Commented-out code
================
Comment at: llvm/lib/Target/M68k/M68kInstrArithmetic.td:164
+ [],
+ MxArithImmEncoding<MxBead4Bits<CMD>,
+ !cast<MxEncSize>("MxEncSize"#TYPE.Size),
----------------
Overindented
================
Comment at: llvm/lib/Target/M68k/M68kInstrArithmetic.td:231
+
+} // isCommutable = ?
+
----------------
isComm, not ?
================
Comment at: llvm/lib/Target/M68k/M68kInstrArithmetic.td:285
+
+} // isCommutable = ?
+
----------------
isComm
================
Comment at: llvm/lib/Target/M68k/M68kInstrArithmetic.td:660
+/// is designed later
+foreach node = ["add", "addc"] in {
+
----------------
`node` is a SelectionDAG type, please call it something else otherwise you're asking for trouble
================
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>;
----------------
jrtc27 wrote:
> Indent.
A lot of these indentation issues are still outstanding from months ago, not hugely impressed by the repeated calls for re-reviews and getting patches landed when there are all these still to be fixed.
================
Comment at: llvm/lib/Target/M68k/M68kInstrControl.td:129
+
+// FIXME Support 16 bit indiret jump.
+// Currently M68k does not allow 16 bit indirect jumps use sext operands
----------------
================
Comment at: llvm/lib/Target/M68k/M68kInstrControl.td:244
+def TAILJMPj : MxPseudo<(outs), (ins MxARI32_TC:$dst)>;
+}
+} // let Uses = [SP]
----------------
jrtc27 wrote:
>
Please mark this one as done to make re-reviewing easier
================
Comment at: llvm/lib/Target/M68k/M68kInstrData.td:49
+ : MxEncoding<
+ srcEA.Reg, srcEA.DA, srcEA.Mode, dstEA.DA, dstEA.Mode, dstEA.Reg, size, MxBead2Bits<0b00>,
+ srcExt.Imm, srcExt.B8, srcExt.Scale, srcExt.WL, srcExt.DAReg,
----------------
Bad formatting
================
Comment at: llvm/lib/Target/M68k/M68kInstrData.td:289
+ !cast<MxType>("MxType"#S#D),
+ MxMoveEncoding<!cast<MxMoveSize>("MxMoveSize"#S),
+ MxEncEAi, !cast<MxEncExt>("MxExtI"#S#"_1"),
----------------
Overindented (confusing)
================
Comment at: llvm/lib/Target/M68k/M68kInstrFormats.td:179-183
+/// If the EA in a direct register mode, bit 4 and 5 are 0, and the register
+/// number will be encoded in bit 0 ~ 3. Since register number of the first
+/// address register (A0) is 8, we can easily tell data register from
+/// address register by only inspecting bit 3 (i.e. if bit 3 is set, it's an
+/// address register).
----------------
(possibly needs reflowing, didn't count the characters)
================
Comment at: llvm/lib/Target/M68k/M68kInstrInfo.td:515-558
+// TODO make it folded like MxType8.F.Op nad MxType8.F.Pat
+// TODO move strings into META subclass
+class MxType<ValueType vt, /// Type of data this fixture refers to
+ string prefix, /// Prefix used to identify type
+ string postfix, /// Prefix used to qualify type
+ string rLet, /// Register letter
+ MxOperand rOp, /// Supported any register operand
----------------
This is some really weird formatting. I mean, it's very readable, but it's definitely not conforming...
================
Comment at: llvm/lib/Target/M68k/M68kRegisterInfo.td:65-71
+// def BP : MxAliasReg<"bp", A5>;
+// def FP : MxAliasReg<"fp", A6>;
+def SP : MxAliasReg<"sp", A7>;
+
+// def USP : MxAliasReg<"usp", A7>;
+// def SSP : MxAliasReg<"ssp", A7>;
+// def ISP : MxAliasReg<"isp", A7>;
----------------
Why are some commented out?
================
Comment at: llvm/lib/Target/M68k/M68kRegisterInfo.td:73
+
+// TODO get rid of "pseudo" registers and use just MxReg variants and use
+// HWEncoding's other 13 bits to encode type(and potentially other info) of
----------------
They already are using MxReg?
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D88389/new/
https://reviews.llvm.org/D88389
More information about the llvm-commits
mailing list