[PATCH] Add a case to LiveIntervalAnalysis::HandleMoveUp
Matthias Braun
mbraun at apple.com
Tue Sep 17 10:40:35 PDT 2013
On Sep 17, 2013, at 8:24 AM, Vincent Lejeune <vljn at ovi.com> wrote:
> Hi Jakob,
>
> Now that R600 backend properly packetizes instructions and supports all vliw modifications in the
> R600 family I have some more reliable test to check correctness of this change.
> I updated the patch ; I didn't find a lot of place to add assert (I'm relying on the LiveInterval::verify() function calls
> made by the MachineScheduler).
You should also use the MachineVerifier (—verify-machineinstrs) as that catches a lot more cases of invalid intervals.
> The patch here : http://cgit.freedesktop.org/~vlj/llvm/commit/?h=vliw5&id=80a1d4220f501afaa714ad076422bd332375e616
> uses these changes and provides some tests.
> The LiveInterval refactoring patch series may be impacted by this patch (the goal of the serie is to track subreg liveness
> which is directly related to what I need) so I'm adding Matthias Braun to the recipient lists.
This doesn't affect the LiveInterval refactoring too much (except some renaming but that should be easy). The stuff happening at the git repo patch should become easier/unnecessary once we have subregister liveness in place…
>
> Vincent
The following 3 new functions aren’t using any of the things in HMEditor, so they can be pulled out and made static.
+ /// Change the Undef flag of def MachineOperand that are subreg of Reg in MI
+ void setReadUndef(MachineInstr *MI, bool Flag, unsigned Reg) {
+ assert(MI && "Resetting undefined MI");
+ for (MachineInstr::mop_iterator MOp = MI->operands_begin(),
+ E = MI->operands_end(); MOp != E; ++MOp) {
+ if (MOp->isReg() && MOp->isDef() &&
+ MOp->getReg() == Reg && MOp->getSubReg()) {
+ MOp->setIsUndef(Flag);
+ }
+ }
+ }
+
/// Update all live ranges touched by MI, assuming a move from OldIdx to
/// NewIdx.
void updateAllRanges(MachineInstr *MI) {
@@ -784,6 +796,48 @@ private:
LI.verify();
}
This splitRange function makes several assumptions that may not always be true:
* It assumes another interval exists at llvm::next(I). Anyway I don’t understand how you can just overwrite that 2nd interval, the liveness data in it will be lost.
* It assumes that the next interval starts with its own value ValNO (otherwise changing it is not correct)
These facts should at least be documented
+ /// Split interval [a, b) at I in two at SplitPos.
+ /// Interval at I becomes [a, SplitPos) and its successor get
+ /// overwritten and becomes [SplitPos, b).
+ /// If KeptValue is false, the second range takes DefVNI value.
+ /// If KeptValue is true, the new first range takes DefVNI value.
+ /// DefVNI def is modified to reflect the change.
+ void splitRange(LiveInterval::iterator I, SlotIndex SplitPos, VNInfo *DefVNI,
+ bool KeptValue) {
+ assert(I->contains(SplitPos) && "SplitPos not inside I");
+ if (KeptValue) {
+ *llvm::next(I) = *I;
+ llvm::next(I)->start = llvm::next(I)->valno->def = SplitPos;
+ *I = LiveRange(I->start, SplitPos, DefVNI);
+ DefVNI->def = I->start;
+ } else {
+ *llvm::next(I) = LiveRange(SplitPos, I->end, DefVNI);
+ I->end = SplitPos;
+ DefVNI->def = SplitPos;
+ }
+ return;
+ }
+
+ /// Merge Range and its successor and write the result to Dst.
+ /// If KeptValue is 0, Dst's value is Range's.
+ /// If KeptValue is 1, Dst's value is Range's successor's.
+ /// Returns the value not used by Dst.
+ VNInfo *mergeRange(LiveInterval::iterator I, LiveRange *Dst, bool KeptValue) {
+ VNInfo *DefVNI = 0;
+ SlotIndex Start = I->start, End = llvm::next(I)->end;
+ if (KeptValue) {
+ DefVNI = I->valno;
+ *Dst = *llvm::next(I);
+ Dst->start = Dst->valno->def = Start;
+ } else {
+ DefVNI = llvm::next(I)->valno;
+ *Dst = *I;
+ Dst->end = End;
+ }
Instead of the if, you could just do:
VNInfo *DefVNI = KeptValue ? I->valno : llvm::next(I)->valno;
*Dst = LiveRange(Start, End, DefVNI);
+ assert(DefVNI && "DefVNI undefined");
+ return DefVNI;
+ }
+
/// Update LI to reflect an instruction has been moved downwards from OldIdx
/// to NewIdx.
///
@@ -804,6 +858,11 @@ private:
/// 5. Value read at OldIdx, killed before NewIdx:
/// Extend kill to NewIdx.
///
+ /// 6. Live def at OldIdx AND several other values between NewIdx and OldIdx
+ /// Extend the range that precedes OldIdx one and split the range that
+ /// contains NewIdx.
+ /// (Happens when reordering independant partial write to a register)
independent
+ ///
void handleMoveDown(LiveInterval &LI) {
// First look for a kill at OldIdx.
LiveInterval::iterator I = LI.find(OldIdx.getBaseIndex());
@@ -818,6 +877,19 @@ private:
// If the live-in value already extends to NewIdx, there is nothing to do.
if (!SlotIndex::isEarlierInstr(I->end, NewIdx))
return;
+ // If value is used at OldIdx and is moved to another interval, it's a
+ // consequence of subreg write reordering.
+ // Check if we need to extend the destination interval and return
+ if (llvm::next(I) != E &&
+ !SlotIndex::isSameInstr(OldIdx, llvm::next(I)->start) &&
+ SlotIndex::isEarlierInstr(llvm::next(I)->start, NewIdx)) {
+ LiveInterval::iterator NewI = LI.advanceTo(I, NewIdx.getBaseIndex());
+ if (NewI != E && SlotIndex::isEarlierInstr(NewI->start, NewIdx))
+ return;
+ NewI = llvm::prior(NewI);
+ NewI->end = NewIdx.getRegSlot(NewI->end.isEarlyClobber());
+ return;
+ }
Wouldn’t it be possible that the interval containing OldIdx has to shrink now?
// Aggressively remove all kill flags from the old kill point.
// Kill flags shouldn't be used while live intervals exist, they will be
// reinserted by VirtRegRewriter.
@@ -850,11 +922,34 @@ private:
// The remaining possibilities are now:
// 2. Live def at OldIdx, killed at NewIdx: isSameInstr(I->end, NewIdx).
// 3. Dead def at OldIdx: I->end = OldIdx.getDeadSlot().
+ // 6. Live def at OldIdx moving accross several others values
// In either case, it is possible that there is an existing def at NewIdx.
- assert((I->end == OldIdx.getDeadSlot() ||
- SlotIndex::isSameInstr(I->end, NewIdx)) &&
- "Cannot move def below kill”);
Maybe that assert could be left in place under the condition that you have no partial register write.
LiveInterval::iterator NewI = LI.advanceTo(I, NewIdx.getRegSlot());
+ if (!SlotIndex::isSameInstr(I->start, I->end) &&
+ SlotIndex::isEarlierInstr(I->end, NewIdx)) {
+ // OldIdx is not a dead def, and NewIdx is inside a new interval.
+ // Case 6 above.
+ const SlotIndex SplitPos = DefVNI->def;
+ if (I != LI.begin() &&
+ !SlotIndex::isEarlierInstr(llvm::prior(I)->end, I->start)) {
+ // There is no gap between I and its predecessor, merge them
+ // (We may fix the extension of the predecessor made in the live in
+ // case)
+ DefVNI = mergeRange(llvm::prior(I), llvm::prior(I), 0);
+ } else {
+ // Value is LiveIn
+ setReadUndef(LIS.getInstructionFromIndex(NewIdx), false, LI.reg);
+ setReadUndef(LIS.getInstructionFromIndex(llvm::next(I)->start), true,
+ LI.reg);
+ I->start = I->valno->def = I->end;
+ DefVNI = mergeRange(I, llvm::next(I), 0);
This seems to assume that llvm::next(I) is the next partial write in a series, Is this always true?
+ }
+ std::copy(llvm::next(I), llvm::next(NewI), I);
+ splitRange(llvm::prior(NewI), SplitPos, DefVNI, 1);
+ return;
+ }
+
+
if (NewI != E && SlotIndex::isSameInstr(NewI->start, NewIdx)) {
// There is an existing def at NewIdx, case 4 above. The def at OldIdx is
// coalesced into that value.
@@ -891,6 +986,11 @@ private:
/// Hoist kill to NewIdx, then scan for last kill between NewIdx and
/// OldIdx.
///
+ /// 6. Live def at OldIdx AND several other values between NewIdx and OldIdx
+ /// Extend the range that precedes OldIdx one and split the range that
+ /// contains NewIdx.
+ /// (Happens when reordering independant partial write to a register)
independent
+ ///
void handleMoveUp(LiveInterval &LI) {
// First look for a kill at OldIdx.
LiveInterval::iterator I = LI.find(OldIdx.getBaseIndex());
@@ -898,7 +998,6 @@ private:
// Is LI even live at OldIdx?
if (I == E || SlotIndex::isEarlierInstr(OldIdx, I->start))
return;
-
// Handle a live-in value.
if (!SlotIndex::isSameInstr(I->start, OldIdx)) {
// If the live-in value isn't killed here, there is nothing to do.
@@ -906,6 +1005,7 @@ private:
return;
// Adjust I->end to end at NewIdx. If we are hoisting a kill above
// another use, we need to search for that use. Case 5 above.
+ SlotIndex NewEnd = NewIdx.getRegSlot(I->end.isEarlyClobber());;
I->end = NewIdx.getRegSlot(I->end.isEarlyClobber());
++I;
// If OldIdx also defines a value, there couldn't have been another use.
@@ -939,6 +1039,29 @@ private:
return;
}
+ if (!SlotIndex::isSameInstr(I->start, I->end) && I != LI.begin() &&
+ SlotIndex::isEarlierInstr(NewIdx, llvm::prior(I)->start)) {
+ // Case 6:
+ // OldIdx is not a dead def and NewIdx is before predecessor start
+ // First we extand range prior to I
extend
+ LiveInterval::iterator NewI = LI.find(NewIdx.getRegSlot(true));
+ const SlotIndex SplitPos = DefVNI->def;
+ DefVNI = mergeRange(llvm::prior(I), I, 1);
+ std::copy_backward(NewI, llvm::prior(I), I);
+ if (SlotIndex::isEarlierInstr(NewI->start, NewIdx)) {
+ splitRange(NewI, SplitPos, DefVNI, 0);
+ } else {
+ // Value become live in
+ setReadUndef(LIS.getInstructionFromIndex(NewIdx), true, LI.reg);
+ setReadUndef(LIS.getInstructionFromIndex(llvm::next(NewI)->start),
+ false, LI.reg);
+ *NewI = *llvm::next(NewI);
+ NewI->start = NewI->valno->def = SplitPos;
+ splitRange(NewI, llvm::next(NewI)->start, DefVNI, 0);
+ }
+ return;
+ }
+
// There is no existing def at NewIdx. Hoist DefVNI.
if (!I->end.isDead()) {
// Leave the end point of a live def.
--
1.8.3.1
Greetings,
Matthias
>
>
>
>
>
> ----- Mail original -----
>> De : Jakob Stoklund Olesen <stoklund at 2pi.dk>
>> À : Vincent Lejeune <vljn at ovi.com>
>> Cc : Andrew Trick <atrick at apple.com>; Commit Messages and Patches for LLVM <llvm-commits at cs.uiuc.edu>
>> Envoyé le : Lundi 15 avril 2013 20h42
>> Objet : Re: [PATCH] Add a case to LiveIntervalAnalysis::HandleMoveUp
>>
>>
>> On Apr 14, 2013, at 7:03 AM, Vincent Lejeune <vljn at ovi.com> wrote:
>>
>>> Hi Jakob,
>>>
>>> can I have a review of the first patch ? Apparently It doesn't bring
>> regressions,
>>> and I think I handled Live out value the way you mentionned in your
>> previous mail.
>>
>> Hi Vincent,
>>
>> This construct confuses me a bit:
>>
>> + switch (KeptValue) {
>> + case 0:
>> ...
>> + case 1:
>> …
>> + default:
>> + llvm_unreachable("Wrong KeptValue");
>> + }
>>
>> Why not pass a bool? Better yet, since you're always passing a constant
>> KeptValue, why not have different functions?
>>
>> Please don't use pointers as if they were iterators.
>>
>> Please use proper punctuation in comments.
>>
>> I am also concerned about the lack of assertions, it seems unlikely that this
>> code is unable to produce invalid intervals.
>>
>> Thanks,
>> /jakob
> <0001-Add-a-case-to-LiveIntervalAnalysis-HandleMove.patch>
More information about the llvm-commits
mailing list