[PATCH] D15703: [AVR] Add instruction definitions

[Matt Arsenault via llvm-commits]

arsenm added inline comments.

================
Comment at: lib/Target/AVR/AVRInstrFormats.td:56
@@ +55,3 @@
+
+  let isPseudo = 1;
+}
----------------
I think you should also set isCodeGenOnly as well

================
Comment at: lib/Target/AVR/AVRInstrInfo.td:58
@@ +57,3 @@
+// shift nodes
+def AVRlsl : SDNode<"AVRISD::LSL", SDTIntUnaryOp>;
+def AVRlsr : SDNode<"AVRISD::LSR", SDTIntUnaryOp>;
----------------
These custom shift nodes look strange. It looks like the shift amount is passed in an implicit register, but you aren't adding glue to the node for the copy into it

================
Comment at: lib/Target/AVR/AVRInstrInfo.td:1821
@@ +1820,3 @@
+let Defs = [SP],
+hasSideEffects = 0 in
+def SPWRITE : Pseudo<(outs GPRSP:$dst),
----------------
I would recommend putting things like the instruction bits and implicit uses and defs generally in a new class to avoid huge numbers of let blocks

================
Comment at: lib/Target/AVR/AVRInstrInfo.td:1890
@@ +1889,3 @@
+// all sub instruction variants always writes the carry flag
+def : Pat<(subc GPR8:$src, GPR8:$src2),
+          (SUBRdRr GPR8:$src, GPR8:$src2)>;
----------------
Should the pattern inputs just be i8 instead of the register class?

================
Comment at: lib/Target/AVR/AVRInstrInfo.td:1961
@@ +1960,3 @@
+
+// :FIXME: DAGCombiner produces an shl node after legalization from these seq:
+// BR_JT -> (mul x, 2) -> (shl x, 1)
----------------
Can you add an xfailed test for this


http://reviews.llvm.org/D15703