RFC: Code Gen Change!

I just finished coding up a change to how code generation builds
machine instructions. The change is in
include/llvm/CodeGen/MachineInstrBuilder.h, where when you want to add
a register, you have to specify a long list of booleans indicating if
it's defined, implicit, killed, dead, or early clobbered. I don't know
about you, but it was hard for me to read the source and understand
what was going on without looking at the header file. So, I made these
changes.

Instead of all of the booleans, you pass in a flag that has bits set
to indicate what state the register is in:

namespace RegState {
  enum {
    Define = 0x1,
    Implicit = 0x2,
    Kill = 0x4,
    Dead = 0x8,
    EarlyClobber = 0x10,
    ImplicitDefine = Implicit | Define,
    ImplicitKill = Implicit | Kill
  };
}

class MachineInstrBuilder {
  MachineInstr *MI;
public:
  explicit MachineInstrBuilder(MachineInstr *mi) : MI(mi) {}

  /// addReg - Add a new virtual register operand...
  ///
  const
  MachineInstrBuilder &addReg(unsigned RegNo, unsigned flags = 0,
                              unsigned SubReg = 0) const {
    MI->addOperand(MachineOperand::CreateReg(RegNo,
                                             flags & RegState::Define,
                                             flags & RegState::Implicit,
                                             flags & RegState::Kill,
                                             flags & RegState::Dead,
                                             SubReg,
                                             flags & RegState::EarlyClobber));
    return *this;
  }

To use this, you would do something like:

BuildMI(...).addReg(Reg, RegState::Kill);

etc.

In a lot of cases, an explicit true/false isn't passed into the addReg
method. I added some helper functions to help make this less of a
pain:

inline unsigned getDefRegState(bool B) {
  return B ? RegState::Define : 0;
}
inline unsigned getImplRegState(bool B) {
  return B ? RegState::Implicit : 0;
}
inline unsigned getKillRegState(bool B) {
  return B ? RegState::Kill : 0;
}
inline unsigned getDeadRegState(bool B) {
  return B ? RegState::Dead : 0;
}

So you can use them like this:

BuildMI(...).addReg(Reg, getKillRegState(isKill);

My hope is that this is a cleaner way of building these machine instructions.

Comments and suggestions are welcome.

-bw

Instead of all of the booleans, you pass in a flag that has bits set
to indicate what state the register is in:

namespace RegState {
enum {
   Define = 0x1,
   Implicit = 0x2,
   Kill = 0x4,
   Dead = 0x8,
   EarlyClobber = 0x10,
   ImplicitDefine = Implicit | Define,
   ImplicitKill = Implicit | Kill
};
}

[...]

MachineInstrBuilder &addReg(unsigned RegNo, unsigned flags = 0,
                             unsigned SubReg = 0) const {

Hi Bill,

I definitely like this change. The staccato bool arguments are impossible to read. One comment:

If I forget to update an addReg(Reg, true) call, it will still compile and work by accident. If you leave bit 0 unused by the enum and assert(flags&1 == 0), you make sure I have updated all my addReg calls.

Humans being human, we are going to fix addReg calls until everything compiles and passes 'make check'. I don't think addReg(Reg, true) should be allowed to survive the API change.

/jakob

True. I went over all of the "addReg()" calls and tried to cover all of them. Your suggestion would be a good way to stop errors from sneaking back in. :slight_smile:

-bw