[PING] New SystemZ backend: LLVM code changes

[Chris Lattner]

On Apr 29, 2013, at 6:59 AM, Richard Sandiford <rsandifo at linux.vnet.ibm.com> wrote:
> A ping for the SystemZ port.  Eric has reviewed the configure and test bits,
> so I think those are approved.  Anton has reviewed the lib/Target bits,
> and Uli has addressed his comments, but Anton thought that someone else
> should have a look too.  Thanks to everyone who has commented so far!
> 
> Here's a repost of the bits that I think still need review.  I realise
> this might not be the best time, what with the conference this week,
> but maybe someone would like something to read on their trip back?

This is pretty exciting.  I'm thrilled to see the SystemZ backend come back, and this time being "real" :).  It would be nice to get this in for 3.3.

Some random comments, mostly stylistic, since I'm trusting you guys know the architecture pretty well.  Overall, I'm really impressed how clean this is, and how well it is following best practices in LLVM.  It also makes me sad how much boilerplate is still required of new targets (e.g. so much of SystemZInstrInfo.cpp should be autogenerated by tblgen), but that's not your fault :-)


Please land this in pieces.  For example, these are approved to go in right away:
   - the Support/Triple.cpp stuff
   - support for building on s390 hosts (Support/Unix/Signals.inc etc)

When this target goes in, please update various stuff in llvm/docs and the web page to mention that the target is supported.  This definitely needs an update:
http://llvm.org/docs/CodeGenerator.html#target-specific-implementation-notes

Also, please consider writing a blog post for blog.llvm.org when it goes in (contact me off list).

Overall, the target is looking good to go in.  I have some specific comments that you should consider before committing it though:


In:

+++ b/lib/Target/SystemZ/AsmParser/SystemZAsmParser.cpp
@@ -0,0 +1,693 @@
+//===-- SystemZAsmParser.cpp - Parse SystemZ assembly instructions --==----===//

Please fix that to be:

+//===-- SystemZAsmParser.cpp - Parse SystemZ assembly instructions --------===//




+void SystemZInstPrinter::printCond4Operand(const MCInst *MI, int OpNum,
+                                           raw_ostream &O) {
+  static const char *const CondNames[] = {
+    "o", "h", "nle", "l", "nhe", "lh", "ne",
+    "e", "nlh", "he", "nl", "le", "nh", "no"
+  };

It would be good to hoist this array up into SystemZ.h (or some new more specific header that talks about instruction encodings) near the definition of the cond40 layout definition code.  Similarly things like:

+// Condition-code mask assignments for floating-point comparisons.
+const unsigned CMP_EQ = SystemZ::CCMASK_0;
+const unsigned CMP_LT = SystemZ::CCMASK_1;
+const unsigned CMP_GT = SystemZ::CCMASK_2;
+const unsigned CMP_UN = SystemZ::CCMASK_3;

Otherwise the indexes into the arrays are "magic numbers" in a sense.


+#include "llvm/MC/MCCodeEmitter.h"
+#include "llvm/MC/MCContext.h"
+#include "llvm/MC/MCExpr.h"
+#include "llvm/MC/MCFixup.h"
+#include "llvm/MC/MCInst.h"
+#include "llvm/MC/MCInstrInfo.h"
+#include "llvm/MC/MCRegisterInfo.h"
+#include "llvm/MC/MCSubtargetInfo.h"
+#include "llvm/MC/MCSymbol.h"

Please try to minimize redundant #includes.  I know this isn't straight-forward, but MCInstrInfo.h transitively pulls in at least MCInst.h and MCRegisterInfo.h.



+++ b/lib/Target/SystemZ/SystemZConstantPoolValue.h
...
+class SystemZConstantPoolValue : public MachineConstantPoolValue {

Please add a doxygen comment here explaining what constant pool values you support.

+++ b/lib/Target/SystemZ/SystemZFrameLowering.cpp
@@ -0,0 +1,542 @@
+//==- SystemZFrameLowering.h - Frame lowering for SystemZ ------------------==//

Wrong filename in the common on the first line of the .cpp file.


+void SystemZRegisterInfo::
+eliminateCallFramePseudoInstr(MachineFunction &MF, MachineBasicBlock &MBB,
+                              MachineBasicBlock::iterator MI) const {
+  MBB.erase(MI);
+}

Should this just be the default implementation in the target-independent code?



+def R0D  : GPR64< 0,  "r0",  R0W>, DwarfRegNum<[0]>;
+def R1D  : GPR64< 1,  "r1",  R1W>, DwarfRegNum<[1]>;
+def R2D  : GPR64< 2,  "r2",  R2W>, DwarfRegNum<[2]>;
+def R3D  : GPR64< 3,  "r3",  R3W>, DwarfRegNum<[3]>;
+def R4D  : GPR64< 4,  "r4",  R4W>, DwarfRegNum<[4]>;
+def R5D  : GPR64< 5,  "r5",  R5W>, DwarfRegNum<[5]>;
+def R6D  : GPR64< 6,  "r6",  R6W>, DwarfRegNum<[6]>;

Can this sort of thing use the tblgen "for loop"?


+class SystemZSelectionDAGInfo : public TargetSelectionDAGInfo {
+public:
+  explicit SystemZSelectionDAGInfo(const SystemZTargetMachine &TM);
+  ~SystemZSelectionDAGInfo();
+};

Do you need to define this class at all?  It doesn't do anything.


+/// SystemZ Code Generator Pass Configuration Options.
+class SystemZPassConfig : public TargetPassConfig {

Likewise, can you just use TargetPassConfig directly?


-Chris