[PATCH] D17011: [lanai] Add Lanai backend.

[Tim Northover via llvm-commits]

t.p.northover added a comment.

Hi again,

Thanks for doing the updates. I think this is looking pretty good now, hopefully even the last round of comments. Mostly trivia, with just one substantial issue over shifts.

Cheers.

Tim.


================
Comment at: lib/Target/AMDGPU/AsmParser/AMDGPUAsmParser.cpp:496
@@ -495,2 +495,3 @@
 
+  std::unique_ptr<AMDGPUOperand> parseRegister();
   bool ParseRegister(unsigned &RegNo, SMLoc &StartLoc, SMLoc &EndLoc) override;
----------------
The changes to this file aren't intentional, are they?

================
Comment at: lib/Target/Lanai/AsmParser/LanaiAsmParser.cpp:368
@@ +367,3 @@
+    // Add as immediates where possible. Null MCExpr = 0
+    if (Expr == 0) {
+      Inst.addOperand(MCOperand::createImm(0));
----------------
nullptr?

================
Comment at: lib/Target/Lanai/LanaiInstrInfo.td:429-431
@@ +428,5 @@
+  let H = 0 in
+    def SHL_I : ShiftRI<"sh", 0, shl, immZExt6>;
+  let H = 1 in
+    def SHA_I : ShiftRI<"sha", 0, shl, immZExt6>;
+  let H = 0 in
----------------
I think the shifts are a bit messy (especially the immZExt6 and immNegZext6 Operands which don't seem to agree between any 2 places they're mentioned).

The comments in the formats file say that imm16 is sign extended, and only values between -31 and 31 are valid.

What I'd suggest is unifying SHL_I/SRL_I and SHA_I/SRA_I. Their joint operand (called shift_imm, say, because it's not really a 2s-complement imm6 in any sense) then just needs to check "Imm >= -31 && Imm <= 31". You might want to check validity in the decoder, but other than that weird helpers ought to be unnecessary.

This range works for CodeGen too, based on the shl node (negative shifts get weird, but they're UB anyway).

Finally, you'll need separate "def : Pat" patterns to handle srl/sra DAGs.

================
Comment at: lib/Target/Lanai/LanaiInstrInfo.td:728-729
@@ +727,4 @@
+let isCall = 1, hasDelaySlot = 1, isCodeGenOnly = 1, Uses = [SP] in {
+  def CALL : Pseudo<(outs), (ins CallTarget:$addr), "call\t$addr", []>;
+  def CALLR : Pseudo<(outs), (ins GPR:$Rs1), "bt\t$Rs1",
+                     [(Call GPR:$Rs1)]>;
----------------
Even the asm can probably be empty here: if these instructions ever get printed something has gone pretty wrong and you'd probably prefer an error.

================
Comment at: lib/Target/Lanai/MCTargetDesc/LanaiAsmBackend.cpp:164
@@ +163,3 @@
+                                          StringRef CPU) {
+  if (TheTriple.isOSDarwin())
+    assert(0 && "Mac not supported on Lanai");
----------------
So, what do you really support? This seems like it might be an ad-hoc `if(!TheTriple.isOSBinFormatELF())`, if the BSDs are OK.

================
Comment at: lib/Target/Lanai/MCTargetDesc/LanaiMCTargetDesc.cpp:70
@@ +69,3 @@
+                                    MCCodeEmitter *Emitter, bool RelaxAll) {
+  if (T.isOSDarwin()) {
+    llvm_unreachable("Lanai does not support Darwin MACH-O format");
----------------
Similarly isOSBinFormatELF here.


http://reviews.llvm.org/D17011