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
On 13/05/2009, at 02.46, Bill Wendling wrote:> 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
On May 12, 2009, at 10:10 PM, Jakob Stoklund Olesen wrote:> On 13/05/2009, at 02.46, Bill Wendling wrote: >> 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. >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. :-) -bw