[llvm] r224806 - LiveInterval: Introduce createMainRangeFromSubranges().
Matthias Braun
matze at braunis.de
Wed Jan 7 15:36:20 PST 2015
> 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.
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