[PATCH] Add a case to LiveIntervalAnalysis::HandleMoveUp
Vincent Lejeune
vljn at ovi.com
Wed Sep 18 10:22:32 PDT 2013
Hi Matthias,
I expanded the mergeRange and splitRange function because they made the logic less understandable and they didn't
spare a lot of lines.
----- Mail original -----
> De : Matthias Braun <mbraun at apple.com>
> À : Vincent Lejeune <vljn at ovi.com>
> Cc : Jakob Stoklund Olesen <stoklund at 2pi.dk>; Andrew Trick <atrick at apple.com>; Commit Messages and Patches for LLVM <llvm-commits at cs.uiuc.edu>
> Envoyé le : Mardi 17 septembre 2013 19h40
> Objet : Re: [PATCH] Add a case to LiveIntervalAnalysis::HandleMoveUp
>
>
>
> 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?
I'm assuming that subreg reordering happens in the same basic block,
so intervals between OldIdx's one and NewIdx's one are contigous.
In the context of the patch hunk OldIdx is a use and not a def so OldIdx should not be at the end of its interval here because the end of OldIdx's interval
is the definition of another subreg.
>
> // 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.
ok, the attached patch still remove it but I will try to keep the assertion
in a next version of this patch
>
> 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?
Expanding splitRange makes this clearer, but yes, we're moving down
a def across at least one other range which represents a value in the
same basic block.
>
> + }
> + 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>
>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: 0001-Add-a-case-to-LiveIntervalAnalysis-HandleMove.patch
Type: text/x-patch
Size: 8463 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20130918/b99993aa/attachment.bin>
More information about the llvm-commits
mailing list