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

[Tim Northover via llvm-commits]

t.p.northover added a subscriber: t.p.northover.
t.p.northover added a comment.

Hi,

Sorry it took so long to take a look here.

This seems like a fairly straightforward backend commit, and most of the code looks fine. My comments are generally trivia.

I think the biggest issue is that the CodeGen test coverage is fairly weak. Missing facets I saw:

- Call/return handling.
- Frame lowering.
- Stack handling.
- The custom passes you've added.
- Relocations and ELF output (including large & small memory models).
- Subtarget features, probably.

Cheers.

Tim.


================
Comment at: lib/Target/Lanai/AsmParser/LanaiAsmParser.cpp:682
@@ +681,3 @@
+  unsigned RegNum;
+  switch (Lexer.getKind()) {
+  case AsmToken::Percent:
----------------
This doesn't really look like a switch kind of control flow.

================
Comment at: lib/Target/Lanai/AsmParser/LanaiAsmParser.cpp:712-713
@@ +711,4 @@
+
+  switch (Lexer.getKind()) {
+  case AsmToken::Identifier: {
+    StringRef Identifier;
----------------
Similarly here, I think we'd mostly use an early exit to reduce indentation.

    if (Lexer.getKind() != AsmToken::Identifier)
      return 0;

================
Comment at: lib/Target/Lanai/AsmParser/LanaiAsmParser.cpp:1002
@@ +1001,3 @@
+LanaiAsmParser::parseOperand(OperandVector *Operands, StringRef Mnemonic) {
+  std::unique_ptr<LanaiOperand> Op = 0;
+
----------------
nullptr?

================
Comment at: lib/Target/Lanai/AsmParser/LanaiAsmParser.cpp:1153
@@ +1152,3 @@
+  // First operand is token for instruction
+  StringRef Mnemonic = splitMnemonic(Name, NameLoc, &Operands);
+
----------------
Is the assembler intentionally case-sensitive?

================
Comment at: lib/Target/Lanai/InstPrinter/LanaiInstPrinter.cpp:81
@@ +80,3 @@
+
+bool LanaiInstPrinter::printMemoryStoreIncrement(const MCInst *MI,
+                                                 raw_ostream &Ostream,
----------------
I'd suggest factoring out the common code between the load/store cases, they're almost identical.

================
Comment at: lib/Target/Lanai/Lanai.td:32
@@ +31,3 @@
+def FeatureConditionalAlu
+  : SubtargetFeature<"calu", "has_conditional_alu_", "true",
+                     "Has conditional ALU instructions">;
----------------
LLVM naming convention for variables is CamelCase.

================
Comment at: lib/Target/Lanai/LanaiAluCode.h:45
@@ +44,3 @@
+  // and Make* utility functions
+  PRE_OP_ = 0x40,
+  POST_OP_ = 0x80,
----------------
Why the trailing underscore?

================
Comment at: lib/Target/Lanai/LanaiAsmPrinter.cpp:162-165
@@ +161,6 @@
+
+  switch (Opcode) {
+  default:
+    break;
+  }
+
----------------
Essentially dead code here. Just add the switch if and when it's actually needed.

================
Comment at: lib/Target/Lanai/LanaiCondCode.h:36
@@ +35,3 @@
+// the same code), while only one form can be printed. The parameter
+// use_first_form allows specifying whether the first or
+// second form (in Lanai Instruction Set document) should be used.
----------------
Name mismatch.

================
Comment at: lib/Target/Lanai/LanaiCondCode.h:41
@@ +40,3 @@
+inline static StringRef lanaiCondCodeToString(LPCC::CondCode CC,
+                                              bool UseFirstForm = false) {
+  switch (CC) {
----------------
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.

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

================
Comment at: lib/Target/Lanai/LanaiISelLowering.cpp:996
@@ +995,3 @@
+  SDVTList VTs = DAG.getVTList(Op.getValueType(), MVT::Glue);
+  SmallVector<SDValue, 4> Ops;
+  Ops.push_back(TrueV);
----------------
A basic array is simpler for this kind of use.

================
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 {
----------------
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()`).

================
Comment at: lib/Target/Lanai/LanaiInstrInfo.td:749
@@ +748,3 @@
+// Jump and link
+let isCall = 1, hasDelaySlot = 1, isCodeGenOnly = 1, Uses = [SP, RCA] in {
+  def CALL : InstBR<(outs), (ins CallTarget:$addr), "bt\t$addr", []> {
----------------
Similarly, why does CALL use RCA?

================
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
----------------
(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";
----------------
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).


http://reviews.llvm.org/D17011