[PING] New SystemZ backend: LLVM code changes

[Richard Sandiford]

Thanks for the review!

Chris Lattner <clattner at apple.com> writes:
> +//===-- SystemZAsmParser.cpp - Parse SystemZ assembly instructions
> --==----===//
>
> Please fix that to be:
>
> +//===-- SystemZAsmParser.cpp - Parse SystemZ assembly instructions
> --------===//

Oops, fixed.  I also tweaked some of the other lines that unnecessarily
shortened the //===--...===// sequence.  Some of the .cpp files also had
redundant -*- C++ -*- markers.

> +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.

I doubt this was particularly clear from the code, so just in case:
these CCMASK_* values are actually architected values.  The target has
2 condition-code bits of unfixed meaning.  Conditional branches like
BRANCH RELATIVE ON CONDITION then have a 4-bit mask to say which of the
4 possible condition-code values should trigger a branch.  CCMASK_<x> is
saying which bit of that mask is for condition code value <x>, which is
fixed by the architecture.  In turn, CMP_* is saying how the condition
codes are defined after a comparison.

I agree it would be better to move the CMP_* values to SystemZ, because
other parts of the backend might need to process comparison instructions
in future (done below).  If these assignments were internal LLVM things,
then I'd agree that the array should also be defined in the same place,
to prevent the two from getting out of sync.  However, since this is
really an architecturally-defined enumeration, I'd prefer to keep the
array local if possible.  Nothing that looks at a branch in isolation
should really be attaching a particular meaning to the condition codes;
1 does not always mean "less", etc.  The assembler defines macros like
JH for "jump if higher" as a convenient way of generating BRANCH
RELATIVE ON CONDITION in the common comparison case, but TBH, these
macros can sometimes be a bit confusing after non-comparison operations.

> +#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.

OK, I've tried to cull the unnecessary includes here and elsewhere.
There were quite a few :-)

> +++ b/lib/Target/SystemZ/SystemZConstantPoolValue.h
> ...
> +class SystemZConstantPoolValue : public MachineConstantPoolValue {
>
> Please add a doxygen comment here explaining what constant pool values
> you support.

OK, how does the comment below look?

> +++ 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.

Oops.  I double-checked all other files too.  A couple were missing "MC".

> +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?

This is dead code that I forgot to delete after pulling in r175788,
which moved the function to TargetFrameLowering.  I've deleted it for
real this time.

> +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"?

Looks like it, thanks.

> +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.

Good point... deleted.

> +/// SystemZ Code Generator Pass Configuration Options.
> +class SystemZPassConfig : public TargetPassConfig {
>
> Likewise, can you just use TargetPassConfig directly?

I might be misunderstanding the question, sorry, but I think the
SystemZPassConfig::addInstSelector() definition really is necessary.
The default implementation is just:

  /// addInstSelector - This method should install an instruction selector pass,
  /// which converts from LLVM code to machine instructions.
  virtual bool addInstSelector() {
    return true;
  }

and addPassesToGenerateCode() bails out if that implementation is used:

  // Ask the target for an isel.
  if (PassConfig->addInstSelector())
    return NULL;

Here's a patch with the fixes, relative to the previous one.  I can upload
a full patch if that's more useful, but didn't want to waste too much
bandwidth.

Thanks,
Richard

-------------- next part --------------
A non-text attachment was scrubbed...
Name: systemz-fixes.diff
Type: text/x-patch
Size: 40060 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20130502/a4168bf7/attachment.bin>