On Jul 26, 2007, at 1:18 AM, Christopher Lamb wrote:
> Author: clamb
> Date: Thu Jul 26 03:18:32 2007
> New Revision: 40521
Nice work on subregs!
> + /// LowerSubregs Pass - This pass lowers subregs to register-
> register copies
> + /// which yields suboptimial, but correct code if the register
> allocator
suboptimial -> suboptimal
> +// Returns the Register Class of a physical register
> +static const TargetRegisterClass *getPhysicalRegisterRegClass(
> + const MRegisterInfo &MRI,
> + unsigned reg) {
Please end comments with a period if they are a sentence.
> + assert(MRegisterInfo::isPhysicalRegister(reg) &&
> + "reg must be a physical register");
> + // Pick the register class of the right type that contains this
> physreg.
> + for (MRegisterInfo::regclass_iterator I = MRI.regclass_begin(),
> + E = MRI.regclass_end(); I != E; ++I)
> + if ((*I)->contains(reg))
> + return *I;
> + assert(false && "Couldn't find the register class");
> + return 0;
> +}
> +
> +static bool isSubRegOf(const MRegisterInfo &MRI,
> + unsigned SubReg,
> + unsigned SupReg) {
> + const TargetRegisterDesc &RD = MRI[SubReg];
> + for (const unsigned *reg = RD.SuperRegs; *reg != 0; ++reg)
> + if (*reg == SupReg)
> + return true;
> +
> + return false;
> +}
Would it make sense for getPhysicalRegisterRegClass/isSubRegOf to be
methods on MRegisterInfo?
> +/// runOnMachineFunction - Reduce subregister inserts and extracts
> to register
> +/// copies.
> +///
> +bool LowerSubregsInstructionPass::runOnMachineFunction
> (MachineFunction &MF) {
> + DOUT << "Machine Function\n";
> + const TargetMachine &TM = MF.getTarget();
> + const MRegisterInfo &MRI = *TM.getRegisterInfo();
> +
> + bool MadeChange = false;
> +
> + DOUT << "********** LOWERING SUBREG INSTRS **********\n";
> + DOUT << "********** Function: " << MF.getFunction()->getName()
> << '\n';
> +
> + for (MachineFunction::iterator mbbi = MF.begin(), mbbe = MF.end();
> + mbbi != mbbe; ++mbbi) {
> + for (MachineBasicBlock::iterator mi = mbbi->begin(), me = mbbi-
> >end();
> + mi != me; ++mi) {
> +
> + if (mi->getOpcode() == TargetInstrInfo::EXTRACT_SUBREG) {
...
> +
> + DOUT << "\n";
> + mbbi->erase(mi);
> + MadeChange = true;
> +
> + } else if (mi->getOpcode() == TargetInstrInfo::INSERT_SUBREG) {
...
> +
> + DOUT << "\n";
> + mbbi->erase(mi);
> + MadeChange = true;
> + }
> + }
> + }
> +
> + return MadeChange;
> +}
This loop is reading from invalidated iterators. Specifically, after
you erase the machine instrs, the next iteration of the loop
increments the "mi" iterator, which points to the deleted instruction.
I suggest structuring the loop like this:
for (MachineFunction::iterator mbbi = MF.begin(), mbbe = MF.end();
mbbi != mbbe; ++mbbi) {
for (MachineBasicBlock::iterator mi = mbbi->begin(), me = mbbi-
>end();
mi != me; ) {
MachineInstr *MI = mi++;
if (MI->getOpcode() == TargetInstrInfo::EXTRACT_SUBREG) {
LowerExtract(MI);
} else if (MI->getOpcode() == TargetInstrInfo::INSERT_SUBREG) {
LowerInsert(MI);
}
}}
By incrementing the iterator early, you avoid the invalidation
problems. Use of methods for the lowering make the code easier to
read but has no functionality change,
-Chris
_______________________________________________
llvm-commits mailing list
llvm-commits@cs.uiuc.edu
http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits
