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

[Jacques Pienaar via llvm-commits]

jpienaar marked 2 inline comments as done.
jpienaar added a comment.

Hi,

Thanks for the comments and suggestions. We can definitely add more tests.

Jacques


================
Comment at: lib/Target/Lanai/AsmParser/LanaiAsmParser.cpp:1153
@@ +1152,3 @@
+  // First operand is token for instruction
+  StringRef Mnemonic = splitMnemonic(Name, NameLoc, &Operands);
+
----------------
t.p.northover wrote:
> Is the assembler intentionally case-sensitive?
Yes. That is a bit strict but that is the preference.

================
Comment at: lib/Target/Lanai/LanaiAluCode.h:45
@@ +44,3 @@
+  // and Make* utility functions
+  PRE_OP_ = 0x40,
+  POST_OP_ = 0x80,
----------------
t.p.northover wrote:
> Why the trailing underscore?
To distinguish it from actual ALU codes. I'll make these separate members outside of the enum instead.

================
Comment at: lib/Target/Lanai/LanaiCondCode.h:41
@@ +40,3 @@
+inline static StringRef lanaiCondCodeToString(LPCC::CondCode CC,
+                                              bool UseFirstForm = false) {
+  switch (CC) {
----------------
t.p.northover wrote:
> This second version seems to be only used in  an assertion, and even that only checks the length of what's returned. I'm not really convinced it's worth the hassle.
Fair point, I agree with you and no one has asked for this legacy form so I'll just remove this.

================
Comment at: lib/Target/Lanai/LanaiCondCode.h:100
@@ +99,3 @@
+
+inline static const CondCode stringToLanaiCondCode(StringRef S) {
+  return StringSwitch<CondCode>(S)
----------------
t.p.northover wrote:
> I think this name is misleading given that it uses `EndsWith`.
Renamed to suffixToLanaiCondCode.

================
Comment at: lib/Target/Lanai/LanaiInstrInfo.cpp:329-330
@@ +328,4 @@
+namespace {
+// LanaiSaveReturnAddress - Add an instruction immediately prior to
+// each call instruction to save RCA (add %pc, 16, %rca).
+struct LanaiSaveReturnAddress : public MachineFunctionPass {
----------------
t.p.northover wrote:
> I think the RCA needs more documentation, and this seems to be its densest point. Here it looks like you're using it as a manual return address, but if so what's the point of the CALL instruction? And why is there only code to push it but no corresponding pop?
> 
> Actually, this seems to be about the only code that refers to it at all, as far as I've found (even via `getRARegister()`).
Indeed. This is in an intermediate state of sorts: originally there was one version that always returned via RCA, now the return addressed is always pushed/popped from the stack. In future we would want to use RCA where possible. I updated the comment. Let me know if this makes sense.

================
Comment at: lib/Target/Lanai/LanaiInstrInfo.td:750
@@ +749,3 @@
+let isCall = 1, hasDelaySlot = 1, isCodeGenOnly = 1, Uses = [SP] in {
+  def CALL : InstBR<(outs), (ins CallTarget:$addr), "bt\t$addr", []> {
+    let DDDI = 0b0000;
----------------
Seems that might have been a left-over from the change in calling convention. Removed.

================
Comment at: lib/Target/Lanai/LanaiTargetMachine.cpp:34
@@ +33,3 @@
+  // Data layout (keep in sync with clang/lib/Basic/Targets.cpp)
+  return "E"        // Big endian
+         "-m:e"     // ELF name manging
----------------
t.p.northover wrote:
> (Not at all seriously). Why do this to us???!?
:-)

================
Comment at: lib/Target/Lanai/MCTargetDesc/LanaiMCExpr.cpp:32
@@ +31,3 @@
+    llvm_unreachable("Invalid kind!");
+  case VK_Lanai_ABS_HI:
+    OS << "hi";
----------------
t.p.northover wrote:
> Is this intentionally mixed & upper case? Yes is an entirely acceptable answer, I just thought it might be an overzealous search/replace (having only just noticed that I did that myself a couple of years ago).
Yes it was intentional but only as we were following Hexagon, MIPS and Sparc here. I don't know if there is a specific convention here that should be following instead.


http://reviews.llvm.org/D17011