[llvm-commits] [patch] Intruction Constraint DestReg!=SrcReg (for review)
Evan Cheng
evan.cheng at apple.com
Thu Feb 8 16:36:22 PST 2007
> Index: lib/CodeGen/LiveIntervalAnalysis.cpp
> ===================================================================
> RCS file: /var/cvs/llvm/llvm/lib/CodeGen/LiveIntervalAnalysis.cpp,v
> retrieving revision 1.204
> diff -u -r1.204 LiveIntervalAnalysis.cpp
> --- lib/CodeGen/LiveIntervalAnalysis.cpp 19 Dec 2006 22:41:21 -0000
> 1.204
> +++ lib/CodeGen/LiveIntervalAnalysis.cpp 5 Feb 2007 21:05:57 -0000
> -354,15 +354,26 @@
> //
> // Keep track of whether we replace a use and/or def
> so that we can
> // create the spill interval with the appropriate range.
Please update comment.
> - mop.setReg(NewVReg);
> -
> - bool HasUse = mop.isUse();
> - bool HasDef = mop.isDef();
> - for (unsigned j = i+1, e = MI->getNumOperands(); j !=
> e; ++j) {
> +
> + bool HasUse = false;
> + bool HasDef = false;
> + int ReadLatency = InstrSlots::USE; //default read latency
> + for (unsigned j = i, e = MI->getNumOperands(); j != e;
> ++j) {
> if (MI->getOperand(j).isReg() &&
> MI->getOperand(j).getReg() == li.reg) {
> MI->getOperand(j).setReg(NewVReg);
Is this right? Aren't you in danger of modifying MO?
> - HasUse |= MI->getOperand(j).isUse();
> +
> + if (MI->getOperand(j).isUse()) {
> + HasUse = true;
> + //get the greatest read latency
> + if (!MI->getOperand(j).isImplicit()){
> + int read_latency = MI->getInstrDescriptor()->
> + getOperandConstraint(j,TOI::READ_LATENCY);
> + if (read_latency != -1 && read_latency >
> ReadLatency)
> + ReadLatency = read_latency;
> + }
> + }
> +
> HasDef |= MI->getOperand(j).isDef();
> }
> }
> @@ -376,16 +387,16 @@
> // the spill weight is now infinity as it
> // cannot be spilled again
> nI.weight = HUGE_VALF;
> -
> + unsigned ValNum = nI.getNextValue(~0U, 0);
> if (HasUse) {
> - LiveRange LR(getLoadIndex(index), getUseIndex(index),
> - nI.getNextValue(~0U, 0));
> + LiveRange LR(getLoadIndex(index),
> + getBaseIndex(index) + ReadLatency + 1,
> ValNum);
> DOUT << " +" << LR;
> nI.addRange(LR);
> }
> if (HasDef) {
> LiveRange LR(getDefIndex(index), getStoreIndex(index),
> - nI.getNextValue(~0U, 0));
> + ValNum);
> DOUT << " +" << LR;
> nI.addRange(LR);
> }
Is this right? I think this is adding live intervals for the spill /
restore instruction?
> @@ -441,6 +441,24 @@
> DOUT << "\t\tregister: "; DEBUG(printRegName(interval.reg));
> LiveVariables::VarInfo& vi = lv_->getVarInfo(interval.reg);
> + // For each kill, get the greatest read latency of the virtual
> register.
> + std::vector<char> ReadLatency(vi.Kills.size());
> + for (unsigned i = 0, e = vi.Kills.size(); i != e; ++i){
> + const TargetInstrDescriptor *instrDescriptor =
> + vi.Kills[i]->getInstrDescriptor();
> + ReadLatency[i] = InstrSlots::USE; //default read latency
> + //find the operands that use the virtual register
> + for (unsigned j = 0, f = vi.Kills[i]->getNumOperands(); j !=
> f; ++j){
> + MachineOperand op = vi.Kills[i]->getOperand(j);
> + if (op.isRegister() && op.isUse() && (op.getReg() ==
> interval.reg)) {
> + int read_latency =
> + instrDescriptor->getOperandConstraint(j,
> TOI::READ_LATENCY);
> + if (read_latency != -1 && read_latency > ReadLatency[i])
> + ReadLatency[i] = (char) read_latency;
> + }
> + }
> + }
Question: is it necessary to calculate ReadLatency up front rather
than calculated it on demand (i.e. the rest of the changes in this
file which use ReadLatency)?
Some nit picks.
1. Please pick identifiers that are more consistent with the rest of
the LLVM code. e.g. You'll find TID is used for TargetInstrDescriptor
all over the place.
2. Please use getUseIndex() instead of InstrSlots::Use.
3. Please "unsigned" instead of "char" for ReadLatency to match
getUseIndex(), etc.
> // Virtual registers may be defined multiple times (due to phi
> // elimination and 2-addr elimination). Much of what we do only
> has to be
> // done once for the vreg. We use an empty interval to detect
> the first
> @@ -467,7 +485,7 @@
> // FIXME: what about dead vars?
> unsigned killIdx;
> if (vi.Kills[0] != mi)
> - killIdx = getUseIndex(getInstructionIndex(vi.Kills[0]))+1;
> + killIdx = getInstructionIndex(vi.Kills[0]) + ReadLatency
> [0] + 1;
> else
> killIdx = defIndex+1;
> @@ -514,7 +532,7 @@
> for (unsigned i = 0, e = vi.Kills.size(); i != e; ++i) {
> MachineInstr *Kill = vi.Kills[i];
> LiveRange LR(getMBBStartIdx(Kill->getParent()),
> - getUseIndex(getInstructionIndex(Kill))+1,
> + getInstructionIndex(Kill)+ ReadLatency[i] + 1,
> ValNum);
> interval.addRange(LR);
> DOUT << " +" << LR;
> @@ -574,7 +592,7 @@
> // Remove the old range that we now know has an incorrect
> number.
> MachineInstr *Killer = vi.Kills[0];
> unsigned Start = getMBBStartIdx(Killer->getParent());
> - unsigned End = getUseIndex(getInstructionIndex(Killer))+1;
> + unsigned End = getInstructionIndex(Killer) + ReadLatency
> [0] + 1;
> DOUT << "Removing [" << Start << "," << End << "] from: ";
> interval.print(DOUT, mri_); DOUT << "\n";
> interval.removeRange(Start, End);
> Index: lib/CodeGen/RegAllocLocal.cpp
> ===================================================================
> RCS file: /var/cvs/llvm/llvm/lib/CodeGen/RegAllocLocal.cpp,v
> retrieving revision 1.100
> diff -u -r1.100 RegAllocLocal.cpp
> --- lib/CodeGen/RegAllocLocal.cpp 1 Feb 2007 05:31:50 -0000 1.100
> +++ lib/CodeGen/RegAllocLocal.cpp 5 Feb 2007 21:05:57 -0000
> @@ -567,10 +567,20 @@
> }
> SmallVector<unsigned, 8> Kills;
> + SmallVector<char, 8> ReadLatency;
> for (unsigned i = 0, e = MI->getNumOperands(); i != e; ++i) {
> MachineOperand& MO = MI->getOperand(i);
> - if (MO.isRegister() && MO.isKill())
> + if (MO.isRegister() && MO.isKill()){
> Kills.push_back(MO.getReg());
> + int read_latency = 1;
> + //we can't call getOperandConstraint on a implicit MO
> + if (!MO.isImplicit()) {
> + read_latency = TID.getOperandConstraint
> (i,TOI::READ_LATENCY);
> + if (read_latency == -1)
> + read_latency = 1;
> + }
> + ReadLatency.push_back((char)read_latency);
> + }
> }
Ok. The only question is whether this needs to be calculated up front
or on demand.
> // Get the used operands into registers. This has the
> potential to spill
> @@ -591,6 +601,7 @@
> // value, freeing the register being used, so it doesn't need
> to be
> // spilled to memory.
> //
> + SmallVector<unsigned, 8> PostponedRegRemove;
> for (unsigned i = 0, e = Kills.size(); i != e; ++i) {
> unsigned VirtReg = Kills[i];
> unsigned PhysReg = VirtReg;
> @@ -606,16 +617,21 @@
> }
> if (PhysReg) {
> - DOUT << " Last use of " << RegInfo->getName(PhysReg)
> - << "[%reg" << VirtReg <<"], removing it from live set
> \n";
> - removePhysReg(PhysReg);
> - for (const unsigned *AliasSet = RegInfo->getAliasSet
> (PhysReg);
> - *AliasSet; ++AliasSet) {
> - if (PhysRegsUsed[*AliasSet] != -2) {
> - DOUT << " Last use of "
> - << RegInfo->getName(*AliasSet)
> - << "[%reg" << VirtReg <<"], removing it from
> live set\n";
> - removePhysReg(*AliasSet);
> + if (ReadLatency[i] > 1) {
> + //postpone the reg remove from live set.
> + PostponedRegRemove.push_back(PhysReg);
> + } else {
> + DOUT << " Last use of " << RegInfo->getName(PhysReg)
> + << "[%reg" << VirtReg <<"], removing it from live
> set\n";
> + removePhysReg(PhysReg);
> + for (const unsigned *AliasSet = RegInfo->getAliasSet
> (PhysReg);
> + *AliasSet; ++AliasSet) {
> + if (PhysRegsUsed[*AliasSet] != -2) {
> + DOUT << " Last use of "
> + << RegInfo->getName(*AliasSet)
> + << "[%reg" << VirtReg <<"], removing it from
> live set\n";
> + removePhysReg(*AliasSet);
> + }
> }
> }
> }
> @@ -731,7 +747,24 @@
> }
> }
> }
> -
> +
> + //Do postponed reg removes from live set
> + for (unsigned i = 0, e = PostponedRegRemove.size(); i != e; +
> +i) {
> + DOUT << " Last use of " << RegInfo->getName
> (PostponedRegRemove[i])
> + <<", removing it from live set (postponed)\n";
> + removePhysReg(PostponedRegRemove[i]);
> + for (const unsigned *AliasSet =
> + RegInfo->getAliasSet(PostponedRegRemove[i]);
> + *AliasSet; ++AliasSet) {
> + if (PhysRegsUsed[*AliasSet] != -2) {
> + DOUT << " Last use of "
> + << RegInfo->getName(*AliasSet)
> + <<", removing it from live set (postponed)\n";
> + removePhysReg(*AliasSet);
> + }
> + }
> + }
> +
> // Finally, if this is a noop copy instruction, zap it.
> unsigned SrcReg, DstReg;
> if (TII.isMoveInstr(*MI, SrcReg, DstReg) && SrcReg == DstReg) {
Does this work if read latency is > 2? If it doesn't, that's ok given
the limitation of RegAllocLocal. But please add comments about it.
Also, what happens if the last MI of the MBB has a read latency > 1?
I trust tablegen changes are ok. :-)
Evan
On Feb 6, 2007, at 8:51 AM, Lauro Ramos Venancio wrote:
> READ_LATENCY patch for review.
>
> Sample of use:
> class RegConstraint<string C> {
> string Constraints = C;
> }
>
> // AI_orr - Defines a (op r, r) pattern.
> class AI_orr<string opc, SDNode opnode>
> : AI<(ops GPR:$dst, GPR:$a, GPR:$b),
> !strconcat(opc, " $dst, $a, $b"),
> [(set GPR:$dst, (opnode GPR:$a, GPR:$b))]>,
> RegConstraint<"$a lat 2">;
>
>
> Lauro
>
> 2007/1/26, Evan Cheng <evan.cheng at apple.com>:
>>
>> On Jan 26, 2007, at 1:07 PM, Lauro Ramos Venancio wrote:
>>
>> >> The facility does that have to be that general. There are 4 cycles
>> >> between every two instructions. See LiveIntervalAnalysis:
>> >>
>> >> struct InstrSlots {
>> >> enum {
>> >> LOAD = 0,
>> >> USE = 1,
>> >> DEF = 2,
>> >> STORE = 3,
>> >> NUM = 4
>> >> };
>> >> };
>> >>
>> >> We can restrict the constraint range to 1 - 3. This ensures the
>> last
>> >> use is always the kill while retaining its flexibility.
>> >>
>> >> Evan
>> >
>> > I will try to implement this. Do you have any suggestion for
>> > constraint name and syntax?
>>
>> I don't have any great ideas. Perhaps READ_LATENCY for constraint and
>> something like
>>
>> $src +lat 2
>>
>> for syntax?
>>
>> Feel free to choose something else if you have better ideas.
>>
>> Thanks,
>>
>> Evan
>>
>> >
>> > Lauro
>>
>>
>> <read_latency.patch>
More information about the llvm-commits
mailing list