[llvm] r224806 - LiveInterval: Introduce createMainRangeFromSubranges().
Quentin Colombet
qcolombet at apple.com
Wed Jan 7 15:39:35 PST 2015
On Jan 7, 2015, at 3:36 PM, Matthias Braun <matze at braunis.de> wrote:
>
>> On Jan 6, 2015, at 5:30 PM, Quentin Colombet <qcolombet at apple.com> wrote:
>>
>> Hi Matthias,
>>
>> Thanks for adding this new capability.
>>
>> I made a few comments to ease readability of the whole code base, nothing correctness related. Feel free to fix them (or argue against them :P) whenever you have time.
>
> Thanks a lot for the review, they were all good suggestions (I hope you don't get too annoyed by me arguing all the time :-). I did not implement factoring out the first loop into a function though, as it shares a lot of state (segment iterator set, Event, EventMask, IsDef) making the code harder to read when I extract a function IMO.
SGTM.
Thanks for the follow-up!
Q.
>
> Greetings
> Matthias
>
>>
>> Thanks,
>> -Quentin
>>
>> On Dec 23, 2014, at 6:11 PM, Matthias Braun <matze at braunis.de> wrote:
>>
>>> Author: matze
>>> Date: Tue Dec 23 20:11:51 2014
>>> New Revision: 224806
>>>
>>> URL: http://llvm.org/viewvc/llvm-project?rev=224806&view=rev
>>> Log:
>>> LiveInterval: Introduce createMainRangeFromSubranges().
>>>
>>> This function constructs the main liverange by merging all subranges if
>>> subregister liveness tracking is available. This should be slightly
>>> faster to compute instead of performing the liveness calculation again
>>> for the main range. More importantly it avoids cases where the main
>>> liverange would cover positions where no subrange was live. These cases
>>> happened for partial definitions where the actual defined part was dead
>>> and only the undefined parts used later.
>>>
>>> The register coalescing requires that every part covered by the main
>>> live range has at least one subrange live.
>>>
>>> I also expect this function to become usefull later for places where the
>>> subranges are modified in a way that it is hard to correctly fix the
>>> main liverange in the machine scheduler, we can simply reconstruct it
>>> from subranges then.
>>>
>>> Modified:
>>> llvm/trunk/include/llvm/CodeGen/LiveInterval.h
>>> llvm/trunk/lib/CodeGen/LiveInterval.cpp
>>> llvm/trunk/lib/CodeGen/LiveRangeCalc.cpp
>>>
>>> Modified: llvm/trunk/include/llvm/CodeGen/LiveInterval.h
>>> URL: http://llvm.org/viewvc/llvm-project/llvm/trunk/include/llvm/CodeGen/LiveInterval.h?rev=224806&r1=224805&r2=224806&view=diff
>>> ==============================================================================
>>> --- llvm/trunk/include/llvm/CodeGen/LiveInterval.h (original)
>>> +++ llvm/trunk/include/llvm/CodeGen/LiveInterval.h Tue Dec 23 20:11:51 2014
>>> @@ -552,6 +552,10 @@ namespace llvm {
>>> void verify() const;
>>> #endif
>>>
>>> + protected:
>>> + /// Append a segment to the list of segments.
>>> + void append(const LiveRange::Segment S);
>>> +
>>> private:
>>>
>>> iterator addSegmentFrom(Segment S, iterator From);
>>> @@ -685,6 +689,10 @@ namespace llvm {
>>> /// are not considered valid and should only exist temporarily).
>>> void removeEmptySubRanges();
>>>
>>> + /// Construct main live range by merging the SubRanges of @p LI.
>>> + void constructMainRangeFromSubranges(const SlotIndexes &Indexes,
>>> + VNInfo::Allocator &VNIAllocator);
>>> +
>>> /// getSize - Returns the sum of sizes of all the LiveRange's.
>>> ///
>>> unsigned getSize() const;
>>>
>>> Modified: llvm/trunk/lib/CodeGen/LiveInterval.cpp
>>> URL: http://llvm.org/viewvc/llvm-project/llvm/trunk/lib/CodeGen/LiveInterval.cpp?rev=224806&r1=224805&r2=224806&view=diff
>>> ==============================================================================
>>> --- llvm/trunk/lib/CodeGen/LiveInterval.cpp (original)
>>> +++ llvm/trunk/lib/CodeGen/LiveInterval.cpp Tue Dec 23 20:11:51 2014
>>> @@ -300,6 +300,12 @@ LiveRange::extendSegmentStartTo(iterator
>>> return MergeTo;
>>> }
>>>
>>> +void LiveRange::append(const Segment S) {
>>> + // Check that the segment belongs to the back of the list.
>>> + assert(segments.empty() || segments.back().end <= S.start);
>>> + segments.push_back(S);
>>> +}
>>> +
>>> LiveRange::iterator LiveRange::addSegmentFrom(Segment S, iterator From) {
>>> SlotIndex Start = S.start, End = S.end;
>>> iterator it = std::upper_bound(From, end(), Start);
>>> @@ -609,6 +615,214 @@ void LiveInterval::removeEmptySubRanges(
>>> }
>>> }
>>>
>>> +/// Helper function for constructMainRangeFromSubranges(): Search the CFG
>>> +/// backwards until we find a place covered by a LiveRange segment that actually
>>> +/// has a valno set.
>>> +static VNInfo *searchForVNI(const SlotIndexes &Indexes, LiveRange &LR,
>>> + const MachineBasicBlock *MBB,
>>> + SmallPtrSetImpl<const MachineBasicBlock*> &Visited) {
>>> + // We start the search at the end of MBB.
>>> + SlotIndex EndIdx = Indexes.getMBBEndIdx(MBB);
>>> + // In our use case we can't live the area covered by the live segments without
>>> + // finding an actual VNI def.
>>> + LiveRange::iterator I = LR.find(EndIdx.getPrevSlot());
>>> + assert(I != LR.end());
>>> + LiveRange::Segment &S = *I;
>>> + if (S.valno != nullptr)
>>> + return S.valno;
>>> +
>>> + VNInfo *VNI = nullptr;
>>> + // Continue at predecessors (we could even go to idom with domtree available).
>>> + for (const MachineBasicBlock *Pred : MBB->predecessors()) {
>>> + // Avoid going in circles.
>>> + if (Visited.count(Pred))
>>> + continue;
>>> + Visited.insert(Pred);
>>
>> Could be replaced by:
>> if (!Visited.insert(Pred).second)
>> continue;
>>
>>> +
>>> + VNI = searchForVNI(Indexes, LR, Pred, Visited);
>>> + if (VNI != nullptr) {
>>> + S.valno = VNI;
>>> + break;
>>> + }
>>> + }
>>> +
>>> + return VNI;
>>> +}
>>> +
>>> +void LiveInterval::constructMainRangeFromSubranges(
>>> + const SlotIndexes &Indexes, VNInfo::Allocator &VNIAllocator) {
>>> + // The basic observations on which this algorithm is based:
>>> + // - Each Def/ValNo in a subrange must have a corresponding def on the main
>>> + // range, but not further defs/valnos are necessary.
>>> + // - If any of the subranges is live at a point the main liverange has to be
>>> + // live too, conversily if no subrange is live the main range mustn't be
>>> + // live either.
>>> + // We do this by scannig through all the subranges simultaneously creating new
>>> + // segments in the main range as segments start/ends come up in the subranges.
>>> + assert(hasSubRanges());
>>
>> Add a message in the assert (even obvious one are useful :)).
>>
>>> + assert(segments.empty() && valnos.empty() && "expected empty main range");
>>> +
>>> + // Collect subrange, iterator pairs for the walk and determine first and last
>>> + // SlotIndex involved.
>>> + SmallVector<std::pair<const SubRange*, const_iterator>, 4> SRs;
>>> + SlotIndex First;
>>> + SlotIndex Last;
>>> + for (const SubRange &SR : subranges()) {
>>> + if (SR.empty())
>>> + continue;
>>> + SRs.push_back(std::make_pair(&SR, SR.begin()));
>>> + if (!First.isValid() || SR.segments.front().start < First)
>>> + First = SR.segments.front().start;
>>> + if (!Last.isValid() || SR.segments.back().end > Last)
>>> + Last = SR.segments.back().end;
>>> + }
>>> +
>>> + errs() << "Compute: " << *this << "\n";
>>> +
>>> + // Walk over all subranges simultaneously.
>>> + Segment CurrentSegment;
>>> + bool ConstructingSegment = false;
>>> + bool NeedVNIFixup = false;
>>> + unsigned ActiveMask = 0;
>>> + SlotIndex Pos = First;
>>> + while (true) {
>>> + SlotIndex NextPos = Last;
>>> + enum {
>>> + NOTHING,
>>> + BEGIN_SEGMENT,
>>> + END_SEGMENT,
>>> + } Event = NOTHING;
>>
>> Maybe add a comment saying that the EventMask covers the lanes that we will add in the main range for this iteration.
>> I think that having a comment on the purpose of the variable will help reading the code later on.
>>
>>> + unsigned EventMask = 0;
>>> + bool IsDef = false;
>>> + // Find the next begin or end of a subrange segment. Combine masks if we
>>> + // have multiple begins/ends at the same position. Ends take precedence over
>>> + // Begins.
>>
>> I would do a function for this loop so that the code is more compact. I think that would help the high level reading of the algorithm.
>>
>>> + for (auto &SRP : SRs) {
>>> + const SubRange &SR = *SRP.first;
>>> + const_iterator &I = SRP.second;
>>
>> Maybe add a comment before this loop saying something along the line: Drop any segment that we already recorded in the main live-range. The invariant is anything ending before Pos is already in main live-range (unless I am mistaken of course :)).
>>
>>> + while (I != SR.end() &&
>>> + (I->end < Pos ||
>>> + (I->end == Pos && (ActiveMask & SR.LaneMask) == 0)))
>>> + ++I;
>>> + if (I == SR.end())
>>> + continue;
>>> + if ((ActiveMask & SR.LaneMask) == 0 &&
>>> + Pos <= I->start && I->start <= NextPos) {
>>> + // Merge multiple begins at the same position
>>
>> Period at the end of the comment.
>>
>>> + if (I->start == NextPos && Event == BEGIN_SEGMENT) {
>>> + EventMask |= SR.LaneMask;
>>> + IsDef |= I->valno->def == I->start;
>>> + } else if (I->start < NextPos || Event != END_SEGMENT) {
>>> + Event = BEGIN_SEGMENT;
>>> + NextPos = I->start;
>>> + EventMask = SR.LaneMask;
>>> + IsDef = I->valno->def == I->start;
>>> + }
>>> + }
>>> + if ((ActiveMask & SR.LaneMask) != 0 &&
>>> + Pos <= I->end && I->end <= NextPos) {
>>> + // Merge multiple ends at the same position.
>>> + if (I->end == NextPos && Event == END_SEGMENT)
>>> + EventMask |= SR.LaneMask;
>>> + else {
>>> + Event = END_SEGMENT;
>>> + NextPos = I->end;
>>> + EventMask = SR.LaneMask;
>>> + }
>>> + }
>>> + }
>>> +
>>> +#if 1
>>> + errs() << '\t' << (Event == NOTHING ? "nothing "
>>> + : Event == BEGIN_SEGMENT ? "begin "
>>> + : "end ")
>>> + << NextPos << " mask " << ActiveMask << " evmask " << EventMask << " def " << IsDef << "\n";
>>> +#endif
>>> +
>>
>> Create a sub function for this?
>> Something like applySegment perhaps?
>> I found that less critical for the reading though.
>>
>>> + // Advance scan position.
>>> + Pos = NextPos;
>>> + if (Event == BEGIN_SEGMENT) {
>>> + if (ConstructingSegment && IsDef) {
>>> + // Finish previous segment because we have to start a new one.
>>> + CurrentSegment.end = Pos;
>>> + append(CurrentSegment);
>>> + ConstructingSegment = false;
>>> + }
>>> +
>>> + // Start a new segment if necessary.
>>> + if (!ConstructingSegment) {
>>> + // Determine value number for the segment.
>>> + VNInfo *VNI;
>>> + if (IsDef) {
>>> + VNI = getNextValue(Pos, VNIAllocator);
>>> + } else {
>>> + // We have to reuse an existing value number, if we are lucky
>>> + // then we already passed one of the predecessor blocks and determined
>>> + // its value number (with blocks in reverse postorder this would be
>>> + // always true but we have no such guarantee).
>>> + assert(Pos.isBlock());
>>> + const MachineBasicBlock *MBB = Indexes.getMBBFromIndex(Pos);
>>> + // See if any of the predecessor blocks has a lower number and a VNI
>>> + for (const MachineBasicBlock *Pred : MBB->predecessors()) {
>>> + SlotIndex PredEnd = Indexes.getMBBEndIdx(Pred);
>>> + VNI = getVNInfoBefore(PredEnd);
>>> + if (VNI != nullptr)
>>> + break;
>>> + }
>>> + // Def will come later: We have to do an extra fixup pass.
>>> + if (VNI == nullptr)
>>> + NeedVNIFixup = true;
>>> + }
>>> +
>>> + CurrentSegment.start = Pos;
>>> + CurrentSegment.valno = VNI;
>>> + ConstructingSegment = true;
>>> + }
>>> + ActiveMask |= EventMask;
>>> + } else if (Event == END_SEGMENT) {
>>> + assert(ConstructingSegment);
>>> + // Finish segment if no lane is active anymore.
>>> + ActiveMask &= ~EventMask;
>>> + if (ActiveMask == 0) {
>>> + CurrentSegment.end = Pos;
>>> + append(CurrentSegment);
>>> + ConstructingSegment = false;
>>> + }
>>> + } else {
>>> + // We reached the end of the last subranges and can stop.
>>> + assert(Event == NOTHING);
>>> + break;
>>> + }
>>> + }
>>> +
>>> + // We might not be able to assign new valnos for all segments if the basic
>>> + // block containing the definition comes after a segment using the valno.
>>> + // Do a fixup pass for this uncommon case.
>>> + if (NeedVNIFixup) {
>>
>> Sub function?
>>
>>> + SmallPtrSet<const MachineBasicBlock*, 5> Visited;
>>> + for (Segment &S : segments) {
>>> + if (S.valno != nullptr)
>>> + continue;
>>> + // This can only happen at the begin of a basic block.
>>> + assert(S.start.isBlock());
>>
>> Message in assert.
>>
>>> +
>>> + Visited.clear();
>>> + const MachineBasicBlock *MBB = Indexes.getMBBFromIndex(S.start);
>>> + for (const MachineBasicBlock *Pred : MBB->predecessors()) {
>>> + VNInfo *VNI = searchForVNI(Indexes, *this, Pred, Visited);
>>> + if (VNI != nullptr) {
>>> + S.valno = VNI;
>>> + break;
>>> + }
>>> + }
>>> + assert(S.valno != nullptr);
>>
>> Message in assert.
>>
>>> + }
>>> + }
>>> + assert(ActiveMask == 0 && !ConstructingSegment);
>>> + errs() << "Result: " << *this << "\n";
>>> + verify();
>>> +}
>>> +
>>> unsigned LiveInterval::getSize() const {
>>> unsigned Sum = 0;
>>> for (const Segment &S : segments)
>>>
>>> Modified: llvm/trunk/lib/CodeGen/LiveRangeCalc.cpp
>>> URL: http://llvm.org/viewvc/llvm-project/llvm/trunk/lib/CodeGen/LiveRangeCalc.cpp?rev=224806&r1=224805&r2=224806&view=diff
>>> ==============================================================================
>>> --- llvm/trunk/lib/CodeGen/LiveRangeCalc.cpp (original)
>>> +++ llvm/trunk/lib/CodeGen/LiveRangeCalc.cpp Tue Dec 23 20:11:51 2014
>>> @@ -105,8 +105,9 @@ void LiveRangeCalc::calculate(LiveInterv
>>> }
>>> }
>>>
>>> - // Create the def in the main liverange.
>>> - if (MO.isDef())
>>> + // Create the def in the main liverange. We do not have to do this if
>>> + // subranges are tracked as we recreate the main range later in this case.
>>> + if (MO.isDef() && !LI.hasSubRanges())
>>> createDeadDef(*Indexes, *Alloc, LI, MO);
>>> }
>>>
>>> @@ -116,13 +117,17 @@ void LiveRangeCalc::calculate(LiveInterv
>>>
>>> // Step 2: Extend live segments to all uses, constructing SSA form as
>>> // necessary.
>>> - for (LiveInterval::SubRange &S : LI.subranges()) {
>>> + if (LI.hasSubRanges()) {
>>> + for (LiveInterval::SubRange &S : LI.subranges()) {
>>> + resetLiveOutMap();
>>> + extendToUses(S, Reg, S.LaneMask);
>>> + }
>>> + LI.clear();
>>> + LI.constructMainRangeFromSubranges(*Indexes, *Alloc);
>>> + } else {
>>> resetLiveOutMap();
>>> - extendToUses(S, Reg, S.LaneMask);
>>> + extendToUses(LI, Reg, ~0u);
>>> }
>>> -
>>> - resetLiveOutMap();
>>> - extendToUses(LI, Reg, ~0u);
>>> }
>>>
>>>
>>>
>>>
>>> _______________________________________________
>>> llvm-commits mailing list
>>> llvm-commits at cs.uiuc.edu
>>> http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits
>>
>>
>> _______________________________________________
>> llvm-commits mailing list
>> llvm-commits at cs.uiuc.edu
>> http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits
>
More information about the llvm-commits
mailing list