[PATCH] D12191: AAP Backend
Tim Northover via llvm-commits
llvm-commits at lists.llvm.org
Tue Feb 9 12:51:41 PST 2016
t.p.northover added a subscriber: t.p.northover.
t.p.northover added a comment.
Hi Edward,
Sorry I didn't reply sooner. I really should have taken a more active role here.
I've got a bunch of comments below (mostly fairly trivial, but with a few questions in there too, and quite possibly some where I should be told to get lost).
But on larger issues, what are your plans for maintaining this backend? Do you have someone lined up to help out with the work in keeping it going, advising on breakages, reviewing patches (sorry, we're not really in a position to be throwing stones here) etc?
Also, do you plan to setup some kind of runtime build bot, or at have someone running regular tests on the code generated?
It didn't quite fit into any inline area on the diff, but we could probably do with some disassembly tests too.
Cheers.
Tim.
================
Comment at: lib/CodeGen/AsmPrinter/AsmPrinter.cpp:146-147
@@ +145,4 @@
+unsigned AsmPrinter::getPointerSize() const {
+ if (MAI)
+ return MAI->getPointerSize();
+ return TM.getPointerSize();
----------------
I take it this is part of the workaround for function pointers being larger than others, but I didn't quite see how all the pieces fit together in LLVM, and didn't see any tests in this area.
This feels like a hack though, could you try to explain a bit more why it's needed?
================
Comment at: lib/CodeGen/MachineRegisterInfo.cpp:348
@@ +347,3 @@
+bool MachineRegisterInfo::isLiveOut(unsigned Reg) const {
+ for (liveout_iterator I = liveout_begin(), E = liveout_end(); I != E; ++I)
+ if (*I == Reg)
----------------
Range-based adaptors?
================
Comment at: lib/Target/AAP/AAP.h:45
@@ +44,3 @@
+// Various helper methods to define operand ranges used throughout the backend
+static bool inline isImm3(int64_t I) { return (I >= 0) && (I <= 7); }
+static bool inline isImm6(int64_t I) { return (I >= 0) && (I <= 63); }
----------------
You should probably be using isInt<N> from MathExtras.h for the signed one. And I'd actually suggest adding something like "bool isIntUInt<N>(int64_t)" too. We really shouldn't be duplicating these helpers all over LLVM.
================
Comment at: lib/Target/AAP/AAP.h:50
@@ +49,3 @@
+static bool inline isImm12(int64_t I) { return (I >= 0) && (I <= 4095); }
+static bool inline isImm16(int64_t I) { return (I >= -32768) && (I <= 65535); }
+
----------------
Apart from this one you seem to be using isImm for unsigned immediates and isOff for signed ones.
I'd sort of suggest isUImm and isSImm or something more explicit, but can certainly live with Imm vs Off if it's consistently applied in the backend.
================
Comment at: lib/Target/AAP/AAP.h:50
@@ +49,3 @@
+static bool inline isImm12(int64_t I) { return (I >= 0) && (I <= 4095); }
+static bool inline isImm16(int64_t I) { return (I >= -32768) && (I <= 65535); }
+
----------------
t.p.northover wrote:
> Apart from this one you seem to be using isImm for unsigned immediates and isOff for signed ones.
>
> I'd sort of suggest isUImm and isSImm or something more explicit, but can certainly live with Imm vs Off if it's consistently applied in the backend.
This also seems misnamed, even if nothing else changes.
================
Comment at: lib/Target/AAP/AAPAsmPrinter.cpp:81
@@ +80,3 @@
+ // vs
+ // mov.w glb(r1), r2
+ // Otherwise (!) msp430-as will silently miscompile the output :(
----------------
This comment doesn't match the alternative output ("#whatever"). Also, the code above suggests it should be "$r1".
================
Comment at: lib/Target/AAP/AAPCallingConv.td:19
@@ +18,3 @@
+def RetCC_AAP : CallingConv<[
+ // Use the same registers for returns as argument passing.
+ CCIfType<[i16], CCAssignToReg<[R2, R3, R4, R5, R6, R7]>>
----------------
R7 differs between the two.
================
Comment at: lib/Target/AAP/AAPCallingConv.td:38
@@ +37,3 @@
+
+def CSR : CalleeSavedRegs<
+ (add R0, R2, R3, R4, R5, R6, R7, R8, R9, R11, R12, R14, R15, R17,
----------------
I think it's worth commenting on the decisions here: R1 = SP, and trying to have about 66% callee-saved with a possibly varying number of registers.
================
Comment at: lib/Target/AAP/AAPFrameLowering.cpp:121-133
@@ +120,15 @@
+
+bool AAPFrameLowering::spillCalleeSavedRegisters(
+ MachineBasicBlock &MBB, MachineBasicBlock::iterator MI,
+ const std::vector<CalleeSavedInfo> &CSI,
+ const TargetRegisterInfo *TRI) const {
+ return false;
+}
+
+bool AAPFrameLowering::restoreCalleeSavedRegisters(
+ MachineBasicBlock &MBB, MachineBasicBlock::iterator MI,
+ const std::vector<CalleeSavedInfo> &CSI,
+ const TargetRegisterInfo *TRI) const {
+ return false;
+}
+
----------------
These are the default implementations. They can probably be skipped for now.
================
Comment at: lib/Target/AAP/AAPISelDAGToDAG.cpp:126-127
@@ +125,4 @@
+
+static bool isOff3(int64_t Imm) { return (Imm >= -4 && Imm <= 3); }
+static bool isOff10(int64_t Imm) { return (Imm >= -512 && Imm <= 511); }
+
----------------
These should probably be with the others.
================
Comment at: lib/Target/AAP/AAPISelDAGToDAG.cpp:164
@@ +163,3 @@
+ Offset = CurDAG->getTargetConstant(off, dl, MVT::i16);
+ } else if (isSubOffset) {
+ Offset = CurDAG->getTargetConstant(-off, dl, MVT::i16);
----------------
I think this should be an assertion. It makes it clearer that Offset is always set, which is a requirement for returning true.
================
Comment at: lib/Target/AAP/AAPISelDAGToDAG.cpp:191
@@ +190,3 @@
+ bool ret = SelectAddr(Addr, B, O);
+ if (ret && isa<ConstantSDNode>(O)) {
+ int64_t c = dyn_cast<ConstantSDNode>(O)->getSExtValue();
----------------
Thisn't the second check always true here? As far as I can see if SelectAddr succeeds then it always creates a TargetConstant.
================
Comment at: lib/Target/AAP/AAPISelLowering.cpp:69-70
@@ +68,4 @@
+ setOperationAction(ISD::GlobalAddress, MVT::i16, Custom);
+ setOperationAction(ISD::SIGN_EXTEND_INREG, MVT::i1, Expand);
+ setOperationAction(ISD::SIGN_EXTEND_INREG, MVT::i8, Expand);
+
----------------
I'm surprised to see so many types in this list (including below) when only MVT::i16 (and possibly MVT::Other) is actually legal.
The type legaliser mostly just goes about its business without checking these, so you can almost certainly drop most of the i1 & i8 entries.
I'd also be very surprised if more than one of the MVT::i16 and MVT::Other cases actually applied in most cases. For example a SELECT should never have an MVT::Other input or result.
It's more complicated for other functions (e.g. setLoadExtAction), so you should expect more of those to be needed.
================
Comment at: lib/Target/AAP/AAPISelLowering.cpp:311
@@ +310,3 @@
+
+ SmallVector<SDValue, 5> Ops;
+ Ops.push_back(Chain);
----------------
An array is usually simpler here. You don't actually need any SmallVector properties, just that the type is convertible to an ArrayRef<SDValue>. So you more often see code like:
SDValue Ops[] = { Chain, DAG.getConstant(TargetCC, dl, MVT::i16), ... };
================
Comment at: lib/Target/AAP/AAPISelLowering.cpp:815
@@ +814,3 @@
+ AAPCC::CondCode CC = (AAPCC::CondCode)MI->getOperand(5).getImm();
+ unsigned branchOp;
+ switch (CC) {
----------------
Here and throughout, the LLVM style is currently that variables start with capital letters.
================
Comment at: lib/Target/AAP/AAPISelLowering.cpp:816
@@ +815,3 @@
+ unsigned branchOp;
+ switch (CC) {
+ case AAPCC::COND_EQ:
----------------
Isn't this duplicating getBranchOpForCondition? If not, a comment about why it's special would be useful.
================
Comment at: lib/Target/AAP/AAPInstrInfo.cpp:45-47
@@ +44,5 @@
+/// any side effects other than loading from the stack slot.
+unsigned AAPInstrInfo::isLoadFromStackSlot(const MachineInstr *MI,
+ int &FrameIndex) const {
+ return 0;
+}
----------------
More default implementations here. They can probably just be removed until you actually want to do something interesting.
================
Comment at: lib/Target/AAP/AAPInstrInfo.td:125
@@ +124,3 @@
+}
+def const6 : Operand<i16>, ImmLeaf<i16, [{
+ return AAP::isImm6(Imm);
----------------
More unity in naming would be good here too.
================
Comment at: lib/Target/AAP/AAPInstrInfo.td:551-553
@@ +550,5 @@
+let isBranch = 1, isTerminator = 1, isBarrier = 1, usesCustomInserter = 1 in {
+ def BR_CC : Pseudo
+ <(outs), (ins i16imm:$cc, GR64:$lhs, GR64:$rhs, brtarget:$target),
+ "#BR_CC", [(AAPbrcc imm:$cc, GR64:$lhs, GR64:$rhs, bb:$target)]>;
+}
----------------
Why does this need to have a custom inserter? It would seem better to get patterns matching each CC into the actual instructions.
Generally, custom inserters are a last resort for instructions that need to mangle the CFG (like your select implementation).
================
Comment at: lib/Target/AAP/AAPInstrInfo.td:572
@@ +571,3 @@
+// Mark R0 as a def as it is the link register
+let isCall = 1, Uses = [R1], Defs = [R0] in {
+
----------------
I don't think I understand these. By convention you're using R0 as the LR, but that's already covered by $rB. And the instruction itself doesn't do anything to R1 (=SP) does it?
================
Comment at: lib/Target/AAP/AAPInstrInfo.td:597
@@ +596,3 @@
+// Calls are encoded as a branch through the link register
+/*def : Pat<(callflag (i16 imm:$imm), GR64:$rB), (BAL imm:$imm, GR64:$rB)>;*/
+
----------------
Commented code. Should probably be removed.
================
Comment at: lib/Target/AAP/AAPRegisterInfo.td:38
@@ +37,3 @@
+def GR64 : RegisterClass<"AAP", [i16], 16,
+ (add R0, R1, R2, R3, R4, R5, R6, R7, R8, R9, R10, R11, R12,
+ R13, R14, R15, R16, R17, R18, R19, R20, R21, R22, R23, R24, R25,
----------------
We've got register sequences for this kind of thing now:
(add (sequence "R%u", 0, 63))
================
Comment at: lib/Target/AAP/MCTargetDesc/AAPMCCodeEmitter.cpp:213-214
@@ +212,4 @@
+ assert(MO.isExpr());
+ const MCInstrDesc &Desc = MCII.get(MI.getOpcode());
+ assert(Desc.getSize() == 4 && "Cannot encode fixup for short imm6");
+
----------------
Trivial point, but I think this will give an unused variable warning outside Debug mode.
Slightly less trivially, does that check only apply to encodeImm6? There's quite a bit of duplication here; many of the encode* functions look like they could defer to an encodeImmN, passing the fixup needed.
================
Comment at: test/MC/AAP/alu.s:8
@@ +7,3 @@
+; ALU operations with registers
+add $r0, $r0, $r0 ;CHECK: {{ADD_r_short}}
+add $r1, $r2, $r7 ;CHECK: {{ADD_r_short}}
----------------
We normally try to check that the roundtrip has succeeded in this kind of MC test, as well as the actual bitwise encoding produced (via -show-encoding). Surprisingly many things can go wrong there (swapping registers around, decoding random bits of an instruction as an operand, ...).
Checking the actual instruction name too isn't absolutely awful, but does feel like a bit of an implementation detail.
http://reviews.llvm.org/D12191
More information about the llvm-commits
mailing list