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

[Eric Christopher via llvm-commits]

echristo added a subscriber: echristo.
echristo added a comment.

Hi,

Some inline comments here about TargetMachine/Subtarget etc.

In general, btw, if you're planning on using subtargets at all and subtarget features you're going to want to look at ports like AArch64 and X86 for guidance. You're saving some variables off to the side that you don't really need and probably won't ever need. :)

I've pointed out some things inline. I'll take another look after these cleanups :)

Thanks!

-eric


================
Comment at: lib/Target/Lanai/LanaiDelaySlotFiller.cpp:38
@@ +37,3 @@
+  // layout, etc.
+  const LanaiSubtarget *Subtarget;
+  const TargetInstrInfo *TII;
----------------
This appears to be unused past getting TII.

================
Comment at: lib/Target/Lanai/LanaiDelaySlotFiller.cpp:48
@@ +47,3 @@
+  bool runOnMachineBasicBlock(MachineBasicBlock &MBB);
+  bool runOnMachineFunction(MachineFunction &F) override {
+    bool Changed = false;
----------------
MF is the standard variable name for MachineFunctions.

================
Comment at: lib/Target/Lanai/LanaiFrameLowering.h:31
@@ +30,3 @@
+protected:
+  const LanaiSubtarget *STI;
+
----------------
Make this a reference?

================
Comment at: lib/Target/Lanai/LanaiISelDAGToDAG.cpp:50-51
@@ +49,4 @@
+class LanaiDAGToDAGISel : public SelectionDAGISel {
+  // TM - Keep a reference to LanaiTargetMachine.
+  LanaiTargetMachine &TM;
+
----------------
Unused.

================
Comment at: lib/Target/Lanai/LanaiISelDAGToDAG.cpp:53-55
@@ +52,5 @@
+
+  // Subtarget - Keep a pointer to the LanaiSubtarget around so that we can
+  // make the right decision when generating code for different targets.
+  const LanaiSubtarget *Subtarget;
+
----------------
Unused? (In addition the DAG also has a copy of the current Subtarget based on function attributes as well).

================
Comment at: lib/Target/Lanai/LanaiISelLowering.h:63
@@ +62,3 @@
+class LanaiSubtarget;
+class LanaiTargetMachine;
+
----------------
Unused.

================
Comment at: lib/Target/Lanai/LanaiISelLowering.h:105
@@ +104,3 @@
+private:
+  const LanaiSubtarget *Subtarget;
+
----------------
Unused it appears? And you can simplify constructors as well if you don't need it.

================
Comment at: lib/Target/Lanai/LanaiSubtarget.h:63
@@ +62,3 @@
+  const LanaiTargetLowering *getTargetLowering() const override {
+    return &target_lowering_info_;
+  }
----------------
In general: What's with the underscores? :) Please follow the coding convention here (and everywhere).

================
Comment at: lib/Target/Lanai/LanaiSubtarget.h:71
@@ +70,3 @@
+private:
+  virtual void anchor();
+  LanaiFrameLowering frame_lowering_;
----------------
Needed?


http://reviews.llvm.org/D17011