New Iteration of subregister liveness patches
Quentin Colombet
qcolombet at apple.com
Fri Dec 5 10:49:32 PST 2014
Hi Matthias,
Renato did all the heavy work of carefully reviewing this last year and as a result most of my comments are nitpicks or refactoring that we could address in subsequent patches if that is simpler for you.
First a couple of general remarks:
- Use clang-format.
- Add a message in asserts (I have reported that in the first patches, I let you review all of them).
- Use range iterator for the subrange loops.
- Remove brackets for one line blocks (I have reported that in the first patches).
- Use nullptr instead of NULL or 0.
On Nov 18, 2014, at 7:40 PM, Matthias Braun <matze at braunis.de> wrote:
> Hi
>
> attached is a new iteration of my subregister liveness tracking patches. I already submitted these for review last year:
> http://lists.cs.uiuc.edu/pipermail/llvm-commits/Week-of-Mon-20131007/190279.html <http://lists.cs.uiuc.edu/pipermail/llvm-commits/Week-of-Mon-20131007/190279.html>
> http://lists.cs.uiuc.edu/pipermail/llvmdev/2013-October/066207.html <http://lists.cs.uiuc.edu/pipermail/llvmdev/2013-October/066207.html>
>
> I did not want to commit them last year because I wasn't able to completely fix the testsuite failures found in the X86/ARM backends. However as I started working on this topic again, I do not want to hold the patches back any longer and prepare people for what is coming.
>
> The attached patchset has no fundamental changes to the patches proposed last year:
> * Rebased/Adapted to llvm trunk
> * Added and improved comments for the RegisterCoalescer changes
> * Fix several edges cases in the RegisterCoalescer related to LiveIntervals which are partially undefined. With subregister liveness we must sometimes keep IMPLICIT_DEF instructions to not break the verifier, there was also a case where we also have to recompute the main liverange if any of the subregister liverange needed to be recomputed.
> * Various comment improvements/cleanups
>
> If you enable the subregister liveness tracking for X86, ARM, R600 the teststuite state is:
> * X86 shows no regression
> * ARM is down to 2 regressions after a set of followup patches I will present in my next mail
> * R600 shows 1 regression
> with subregister liveness tracking disabled no changes are visible.
>
> Greetings
> Matthias
> <0001-Let-tablegen-compute-maximum-lanemask-for-regs-regcl.patch>
LGTM.
> <0002-Add-function-that-translates-subregister-lane-masks-.patch>
LGTM with a few nitpicks.
+ // are usually the same for many subregisters we can easily combin the steps
+ // by combining the masks.
combin -> combine
+ if (LaneTransforms.size() == 1) {
+ LaneTransforms[0].Mask = ~0u;
+ }
No need for brackets.
+ unsigned Found = ~0u;
+ unsigned SIdx = 0;
There are a couple of places with strange formatting like this. Please use clang-format.
+ OS << " --IdxA; assert(IdxA < " << SubRegIndices.size() << ");\n"
Could you add a simple message like "Index out of bound" in the generated assert.
> <0003-Add-const-version-of-LiveRange-advanceTo.patch>
LGTM.
> <0004-Add-a-covers-operation-to-LiveRange.patch>
LGTM.
> <0005-Add-support-to-track-liveness-of-subregisters-in-a-L.patch>
+ class SubRange : public LiveRange {
+ public:
+ SubRange *Next;
[…]
+ template<typename T>
+ class SingleLinkedListIterator {
+ T *P;
+ public:
+ SingleLinkedListIterator<T>(T *P) : P(P) {}
+ SingleLinkedListIterator<T> &operator++() {
+ P = P->Next;
+ return *this;
+ }
Instead of having a new template for single linked list, could it be possible to reuse the ilist class for definition of SubRange?
- : reg(Reg), weight(Weight) {}
+ : SubRanges(0), reg(Reg), weight(Weight) {}
0 -> nullptr
There are a couple of other places where this should happen.
+ LI.clearSubRanges(); // TODO
What is the plan here? Unless I am mistaken, this is not handled by any subsequent patches.
I am just asking, this should not prevent the whole thing to happen. This is strictly an improvement over the existing framework.
+ assert((Mask & I->LaneMask) == 0);
+ assert((Mask & ~MaxMask) == 0);
[…]
+ assert(covers(*I));
Although this is clear from the code what those are checking, having a message in the assert explaining what is checked would help track down errors or reading the code.
Could you add such messages please?
+ // TODO: do not cheat anymore by simply cleaning all subranges
+ LI.clearSubRanges();
Same question as previously, what is the plan here?
+unsigned MachineRegisterInfo::getMaxLaneMaskForReg(unsigned Reg) const
+{
+ assert(TargetRegisterInfo::isVirtualRegister(Reg));
Maybe a message saying that physical registers are not supported and a comment saying why.
+ unsigned Mask = 0;
+ unsigned MaxMask = MRI->getMaxLaneMaskForReg(Reg);
+ for (LiveInterval::const_subrange_iterator I = LI.subrange_begin(),
+ E = LI.subrange_end(); I != E; ++I) {
+ if ((Mask & I->LaneMask) != 0) {
+ report("Lane masks of sub ranges overlap in live interval", MF, LI);
+ }
+ if ((I->LaneMask & ~MaxMask) != 0) {
+ report("Subrange lanemask is invalid", MF, LI);
+ }
+ Mask |= I->LaneMask;
+ verifyLiveRange(*I, LI.reg, I->LaneMask);
+ if (!LI.covers(*I)) {
+ report("A Subrange is not covered by the main range", MF, LI);
+ }
+ }
Could you think of a way we can refactor the code to share logic with LiveInterval::verify instead of duplicating it?
> <0006-Compute-subregister-ranges-in-LiveIntervalAnalysis.patch>
+ const MachineInstr *MI = MO.getParent();
+ // Find the corresponding slot index.
+ SlotIndex Idx;
+ if (MI->isPHI())
+ // PHI defs begin at the basic block start index.
+ Idx = Indexes->getMBBStartIdx(MI->getParent());
+ else
+ // Instructions are either normal 'r', or early clobber 'e'.
+ Idx = Indexes->getInstructionIndex(MI).getRegSlot(MO.isEarlyClobber());
Create a small function for that taking a const MachineOperand & and returning the index. That way we can avoid duplicating this code between both version of createDeadDefs.
+ assert(Common == S->LaneMask);
Message in the assert like “At this point the sub range should be entirely covered”. That may not be super useful right now, but since this is currently trivially true, we have to consider this assert as a guide for future implementation update and thus, explain what was the intent of the assert here.
+ const TargetRegisterInfo &TRI = *MRI->getTargetRegisterInfo();
+ assert(MRI && Indexes && "call reset() first”);
We should invert the order of those statement. If MRI is nullptr, then this will crash instead of asserting.
// Transfer information from the LiveIn vector to the live ranges.
-void LiveRangeCalc::updateLiveIns() {
+void LiveRangeCalc::updateLiveIns(LiveOutData &LiveOuts) {
Add a comment on what the LiveOuts argument is used for. Indeed having LiveIn in the comment and not LiveOuts may be confusing because one can think this is a typo whereas it refers to the class member and both values are used.
bool findReachingDefs(LiveRange &LR, MachineBasicBlock &KillMBB,
- SlotIndex Kill, unsigned PhysReg);
+ SlotIndex Kill, unsigned PhysReg,
+ LiveOutData &LiveOuts);
[…]
- void updateSSA();
+ void updateSSA(LiveOutData &LiveOuts);
[…]
- void updateLiveIns();
+ void updateLiveIns(LiveOutData &LiveOuts);
[…]
- void extend(LiveRange &LR, SlotIndex Kill, unsigned PhysReg = 0);
+ void extend(LiveRange &LR, SlotIndex Kill, unsigned PhysReg,
+ LiveOutData &LiveOuts);
[…]
- void calculateValues();
+ void calculateValues(LiveOutData &LiveOuts);
Add a comment on the usage of LiveOuts.
> <0007-Update-SubRanges-in-LiveIntervalAnalysis-shrinkToUse.patch>
+bool LiveIntervals::shrinkToUses(LiveInterval::SubRange &SR, unsigned Reg)
There is a lot of duplicated code between this new method and the original shrinkToUses.
I think we can do a better job at reusing the logic, here are my proposals:
+ // Create a new live ranges with only minimal live segments per def.
+ LiveRange NewLR;
+ for (LiveInterval::vni_iterator I = SR.vni_begin(), E = SR.vni_end();
+ I != E; ++I) {
+ VNInfo *VNI = *I;
+ if (VNI->isUnused())
+ continue;
+ NewLR.addSegment(LiveInterval::Segment(VNI->def, VNI->def.getDeadSlot(),
+ VNI));
+ }
Create a method for that loop. Arguments could be LiveRange &, LiveInterval::vni_iterator begin, LiveInterval::vni_iterator end.
+ // Keep track of the PHIs that are in use.
+ SmallPtrSet<VNInfo*, 8> UsedPHIs;
+
+ // Extend intervals to reach all uses in WorkList.
+ while (!WorkList.empty()) {
[…]
+ }
Create a method for this whole loop.
Update computeDeadValues to take iterators, a register, and a bool instead of a LiveInterval.
- The iterators would be used as begin and end for the loop.
- The register will be used in place of li->reg.
- The bool will be used to conditionally execute the else-block. I.e., bool == false for sub registers. (Instead of a bool we may use reg != 0 or something).
+ // Handle dead values.
+ bool CanSeparate = false;
[…]
Call computeDeadValues with the appropriate parameter.
> <0008-Adapt-LiveIntervalAnalysis-handleMove-for-subregiste.patch>
LGTM.
> <0009-Adapt-LiveRangeEdit-eliminateDeadDef-for-subregister.patch>
LGTM.
> <0010-Adapt-LiveIntervalAnalysis-repairIntervalsInRange-to.patch>
LGTM.
> <0011-Add-a-flag-to-enable-disable-subregister-liveness.patch>
LGTM.
> <0012-Introduce-LiveQuery-accessor-for-dead-or-live-out-va.patch>
LGTM.
> <0013-Add-subregister-aware-variants-of-LiveIntervalAnalys.patch>
LGTM.
> <0014-Add-LiveInterval-removeEmptySubRanges.patch>
LGTM with a few nitpicks.
+ /// Removes all subranges without any segments (subranges without segments
+ /// are not considered valid and should only exist temporarily)
Period at the end of a comment.
+ // Skip empty subranges until we find the first nonempty one
Ditto.
> <0015-Preserve-subregister-liveranges-in-register-coalesce.patch>
+ /// A LaneMask to remeber on which subregister live ranges we need to call
remeber -> remember
+ void mergeSubRangeInto(LiveInterval &LI, const LiveRange &ToMerge,
+ unsigned LaneMask, CoalescerPair &CP);
+
+ bool joinLanes(LiveRange &LRange, LiveRange &RRange,
+ const CoalescerPair &CP);
Please add comments on those methods.
+static void addSegmentsWithValNo(LiveRange &Dst, VNInfo *DstValNo,
+ const LiveRange &Src, const VNInfo *SrcValNo)
The code is quite trivial to understand but a comment would be still nice to detail how the arguments are used.
+ // duplicate SubRange for newly merged common stuff
Period at the end of the comment and capital letter at the beginning.
+ // we van reuse the L SubRange
Ditto.
+ LiveInterval::SubRange *CommonRange;
+ if (BRest != 0) {
[…]
+ } else {
+ // we van reuse the L SubRange
+ SB->LaneMask = Common;
+ CommonRange = &*SB;
+ }
[…]
+ AMask &= ~BMask;
+ }
+ if (AMask != 0) {
[...]
If I remember correctly, this is the third time this pattern occurs in the patches. I wonder if we could do something (assuming it make sense) to share this logic somewhere.
Thoughts?
+ LiveRange &LR;
+ unsigned Reg;
+ bool SubRangeJoin;
+ bool TrackSubRegLiveness;
Please document the new members.
- void pruneValues(JoinVals &Other, SmallVectorImpl<SlotIndex> &EndPoints);
+ void pruneValues(JoinVals &Other, SmallVectorImpl<SlotIndex> &EndPoints,
+ bool changeInstrs);
Please document the new argument.
+ // Detect impossible conflicts early.
+ if (!LHSVals.mapValues(RHSVals) || !RHSVals.mapValues(LHSVals))
+ abort();
I believe this should either be “return false” or llvm_unreachable().
> <0016-amend-to-RegisterCoalescer.patch>
LGTM. BTW it fixes some of the comments on the previous patch.
> <0017-Tablegen-erate-lanemasks-for-register-units.patch>
LGTM.
> <0018-LiveIntervalUnion-allow-specification-of-liverange-w.patch>
LGTM.
> <0019-Respect-subregister-liveness-when-allocating-registe.patch>
LGTM, with maybe some code refactoring possible.
- for (MCRegUnitIterator Units(PhysReg, TRI); Units.isValid(); ++Units) {
- DEBUG(dbgs() << ' ' << PrintRegUnit(*Units, TRI));
- Matrix[*Units].unify(VirtReg);
+ if (VirtReg.hasSubRanges()) {
+ for (MCRegUnitMaskIterator Units(PhysReg, TRI); Units.isValid(); ++Units) {
[…]
The structure of the code is the same for the three places we update. I was wondering if we could share the logic. What do you think?
> <0020-Do-not-add-implicit-defs-uses-when-tracking-subregis.patch>
LGTM.
> <0021-Add-MCSubRegIndexIterator.patch>
LGTM.
> <0022-Better-Block-live-in-info-if-subregliveness-is-avail.patch>
LGTM.
> <0023-MachineVerifier-Allow-LiveInterval-segments-to-end-a.patch>
LGTM.
> <0024-MachineVerifier-allow-physreg-use-if-just-a-subreg-i.patch>
LGTM.
+ bool bad = !isReserved(Reg);
Variables name should start with a capital letter.
Thanks,
-Quentin
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20141205/4835b40b/attachment.html>
More information about the llvm-commits
mailing list