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

[Eric Christopher via llvm-commits]

echristo accepted this revision.
echristo added a reviewer: echristo.
echristo added a comment.
This revision is now accepted and ready to land.

Some pretty particularly nitpicky comments. That said, really well done on the port and I'm happy when Tim gives a final thumbs up (don't bother uploading again with the changes I've requested, once you do them I'm fine :)

Thanks!

-eric


================
Comment at: lib/Target/Lanai/AsmParser/LanaiAsmParser.cpp:453-457
@@ +452,7 @@
+    assert(N == 1 && "Invalid number of operands!");
+    if (const MCConstantExpr *ConstExpr = dyn_cast<MCConstantExpr>(getImm())) {
+      Inst.addOperand(MCOperand::createImm(ConstExpr->getValue() & 0xffff));
+    } else {
+      assert(false && "Operand type not supported.");
+    }
+  }
----------------
LLVM style is no braces around single if/if-else statements. Going to be a bit of churn to fix though in your patch.

================
Comment at: lib/Target/Lanai/LanaiISelDAGToDAG.cpp:262-263
@@ +261,4 @@
+  switch (ConstraintCode) {
+  case InlineAsm::Constraint_o: // offsetable        ??
+  case InlineAsm::Constraint_v: // not offsetable    ??
+  default:
----------------
Are you actually using these in a way that you should do something with them?

================
Comment at: lib/Target/Lanai/LanaiMemAluCombiner.cpp:81
@@ +80,3 @@
+  // layout, etc.
+  const TargetInstrInfo *II;
+};
----------------
Probably want TII to match other naming conventions.

================
Comment at: lib/Target/Lanai/LanaiMemAluCombiner.cpp:285
@@ +284,3 @@
+                        const MachineOperand &Base,
+                        const MachineOperand &offset) {
+  // ALU operations have 3 operands
----------------
Offset should probably be capitalized.

================
Comment at: lib/Target/Lanai/LanaiMemAluCombiner.cpp:290-292
@@ +289,5 @@
+
+  MachineOperand *Dest = &AluIter->getOperand(0);
+  MachineOperand *Op1 = &AluIter->getOperand(1);
+  MachineOperand *Op2 = &AluIter->getOperand(2);
+
----------------
Weird that you get them all as pointers and then turn them to references on use? :)

================
Comment at: lib/Target/Lanai/LanaiMemAluCombiner.cpp:407
@@ +406,3 @@
+
+  if (DisableMemAluCombiner)
+    return false;
----------------
Can probably move this to the top of the function.

================
Comment at: lib/Target/Lanai/LanaiRegisterInfo.h:28
@@ +27,3 @@
+struct LanaiRegisterInfo : public LanaiGenRegisterInfo {
+  const TargetInstrInfo &TII;
+
----------------
AFAICT you only use this in one function so you could probably just sink it and get it via the MachineFunction.

================
Comment at: lib/Target/Lanai/LanaiSelectionDAGInfo.cpp:26-27
@@ +25,4 @@
+    MachinePointerInfo DstPtrInfo, MachinePointerInfo SrcPtrInfo) const {
+  // This requires the copy size to be a constant, preferably
+  // within a subtarget-specific limit.
+  ConstantSDNode *ConstantSize = dyn_cast<ConstantSDNode>(Size);
----------------
Odd comment when you don't check a subtarget :)

================
Comment at: lib/Target/Lanai/LanaiSetflagAluCombiner.cpp:56-58
@@ +55,5 @@
+
+  // Target machine description which we query for register names, data
+  // layout, etc.
+  const TargetInstrInfo *TII;
+};
----------------
You only seem to use this once in the file. Can probably just sink it.

================
Comment at: lib/Target/Lanai/LanaiSubtarget.h:17
@@ +16,3 @@
+
+#include <string>
+
----------------
Goes after the LLVM includes.

================
Comment at: lib/Target/Lanai/LanaiTargetMachine.cpp:108
@@ +107,3 @@
+void LanaiPassConfig::addPreSched2() {
+  if (TM->getOptLevel() != CodeGenOpt::None) {
+    addPass(createLanaiMemAluCombinerPass());
----------------
I'd probably sink this into the passes themselves. You can set different optimization levels on the function (Attribute::OptimizeNone, for example) and this is just checking the overall module optimization level.

================
Comment at: lib/Target/Lanai/LanaiTargetObjectFile.cpp:113
@@ +112,3 @@
+bool LanaiTargetObjectFile::isConstantInSmallSection(
+    const DataLayout &DL, const Constant *CN, const TargetMachine &TM) const {
+  return isInSmallSection(DL.getTypeAllocSize(CN->getType()));
----------------
TM is unused here...

================
Comment at: lib/Target/Lanai/LanaiTargetObjectFile.cpp:121
@@ +120,3 @@
+                                                        unsigned &Align) const {
+  if (isConstantInSmallSection(DL, C, *TM))
+    return SmallDataSection;
----------------
... and so doesn't need to be passed here, which means you don't need it on the class as every other function passes it down.


http://reviews.llvm.org/D17011