[PATCH] D21189: Create subranges for new intervals resulting from live interval splitting
Matthias Braun via llvm-commits
llvm-commits at lists.llvm.org
Fri Aug 19 13:57:43 PDT 2016
MatzeB added a comment.
This is getting really close. I have no highlevel concerns anymore.
A bunch of smaller changes below (I realize that a number of them just clean up aspects of my "rough" patch).
I also tested the code on an important out-of-tree target with subregister usage here and it worked nicely (though admittedly you rarely see spill instructions there).
================
Comment at: include/llvm/CodeGen/LiveInterval.h:458
@@ -453,1 +457,3 @@
+ VNInfo *extendInBlock(SlotIndex StartIdx, SlotIndex Kill);
+
----------------
This needs documentation. Probably most of the following function. (You can probably shorten the docu of the following function to something like "variant of extendInBlock() ... put undefs explanation here..."
================
Comment at: include/llvm/CodeGen/LiveInterval.h:611
@@ -583,3 +610,3 @@
void markValNoForDeletion(VNInfo *V);
-
+ void pruneUndefs();
};
----------------
this can be removed now.
================
Comment at: include/llvm/CodeGen/LiveIntervalAnalysis.h:166-176
@@ -165,9 +165,13 @@
- /// extendToIndices - Extend the live range of LI to reach all points in
- /// Indices. The points in the Indices array must be jointly dominated by
- /// existing defs in LI. PHI-defs are added as needed to maintain SSA form.
+ /// Extend the live range of LI to reach all points in @p Indices. The
+ /// points in the @p Indices array must be jointly dominated by existing
+ /// defs in @p LR, unless @p AllowUndef is set. If @p AllowUndef is set,
+ /// an attempt to extend @p LR to an index where @p LR is undefined has
+ /// no effect. This is useful when extending subranges, but increases
+ /// the complexity of the extension code.
+ /// PHI-defs are added as needed to maintain SSA form.
///
- /// If a SlotIndex in Indices is the end index of a basic block, LI will be
- /// extended to be live out of the basic block.
+ /// If a SlotIndex in @p Indices is the end index of a basic block, @p LR
+ /// will be extended to be live out of the basic block.
///
/// See also LiveRangeCalc::extend().
----------------
This should not talk about AllowUndef anymore
================
Comment at: lib/CodeGen/LiveInterval.cpp:62-63
@@ -61,2 +61,4 @@
- VNInfo *createDeadDef(SlotIndex Def, VNInfo::Allocator &VNInfoAllocator) {
+ // The value to be defined at Def may be explicitly specified by ForVNI.
+ // In such case, it will be used instead of allocating a new one.
+ VNInfo *createDeadDef(SlotIndex Def, VNInfo::Allocator *VNInfoAllocator,
----------------
Use /// doxygen. Should explain what the function does or use `\see LiveRange::createDeadDef`.
================
Comment at: lib/CodeGen/LiveInterval.cpp:532-533
@@ -500,3 +531,4 @@
// Otherwise use the segment vector.
- return CalcLiveRangeUtilVector(this).addSegment(S);
+ LiveRange::iterator I = CalcLiveRangeUtilVector(this).addSegment(S);
+ return I;
}
----------------
this change appears unnecessary.
================
Comment at: lib/CodeGen/LiveInterval.cpp:542-544
@@ -509,4 +541,5 @@
/// extendInBlock - If this range is live before Kill in the basic
/// block that starts at StartIdx, extend it to be live up to Kill and return
/// the value. If there is no live range before Kill, return NULL.
+std::pair<VNInfo*,bool> LiveRange::extendInBlock(ArrayRef<SlotIndex> Undefs,
----------------
This duplicate comment should be removed, there is one in the header anyway (and this is not up to date anymore after this patch).
================
Comment at: lib/CodeGen/LiveIntervalAnalysis.cpp:581
@@ -579,2 +580,3 @@
LRCalc->reset(MF, getSlotIndexes(), DomTree, &getVNInfoAllocator());
+ SmallVector<SlotIndex, 0> Undefs;
for (unsigned i = 0, e = Indices.size(); i != e; ++i)
----------------
`ArrayRef<SlotIndex> NoUndefs;` should be slightly more efficient and a better name.
================
Comment at: lib/CodeGen/LiveIntervalAnalysis.cpp:979-986
@@ -975,2 +978,10 @@
updateRegMaskSlots();
+
+ for (MachineOperand &MO : MI->operands()) {
+ if (!MO.isReg() || !MO.isDef())
+ continue;
+ unsigned Reg = MO.getReg();
+ if (!TargetRegisterInfo::isVirtualRegister(Reg))
+ continue;
+ }
}
----------------
empty loop?
================
Comment at: lib/CodeGen/LiveRangeCalc.cpp:58-69
@@ -55,1 +57,14 @@
+ auto getCommonRange = [&LI,this] (LiveInterval::SubRange &S,
+ LaneBitmask Mask) {
+ LaneBitmask CM = S.LaneMask & Mask;
+ // A Mask for subregs covered by the subrange but not the current def.
+ if (LaneBitmask RM = S.LaneMask & ~Mask) {
+ // Split current subrange into Common and LRest ranges.
+ S.LaneMask = RM;
+ return LI.createSubRangeFrom(*Alloc, CM, S);
+ }
+ assert(CM == S.LaneMask);
+ return &S;
+ };
+
----------------
this can probably be inlined again now that is only used once.
================
Comment at: lib/CodeGen/LiveRangeCalc.cpp:249
@@ +248,3 @@
+ assert(TargetRegisterInfo::isVirtualRegister(Reg));
+ unsigned VRegMask = MRI->getMaxLaneMaskForVReg(Reg);
+ assert((VRegMask & LaneMask) != 0);
----------------
Should use `LaneBitmask` instead of `unsigned`.
================
Comment at: lib/CodeGen/LiveRangeCalc.h:27
@@ -26,2 +26,3 @@
#include "llvm/ADT/IndexedMap.h"
+#include "llvm/ADT/SmallSet.h"
#include "llvm/CodeGen/LiveInterval.h"
----------------
I believe llvm/ADT/ArrayRef.h is enough here.
================
Comment at: lib/CodeGen/LiveRangeCalc.h:134-140
@@ -112,1 +133,9 @@
///
+ /// Unless @p AllowUndef is set, it is assumed that @p LR is live at @p Kill.
+ /// If @p AllowUndef is set and @p Kill can be reached from entry without
+ /// encountering any def, the function returns false. Allowing @p Kill to be
+ /// undefined helps when extending subranges of a live interval: while the
+ /// register as a whole may be live at @p Kill, a particular subregister may
+ /// be undefined. Whether that is the case or not is often unknown until the
+ /// reaching defs are actually located.
+ ///
----------------
This needs an update now that we have the Undefs list.
================
Comment at: lib/CodeGen/LiveRangeCalc.h:160-164
@@ -129,4 +159,7 @@
///
- /// All uses must be jointly dominated by existing liveness. PHI-defs are
- /// inserted as needed to preserve SSA form.
- void extendToUses(LiveRange &LR, unsigned Reg, LaneBitmask LaneMask);
+ /// All uses must be jointly dominated by existing liveness, unless
+ /// @p AllowUndef is true.. PHI-defs are inserted as needed to preserve
+ /// SSA form. The set @p LiveAt optionally contains indexes where the
+ /// register is known to be live. The motivation for this are read-undef
+ /// definitions of Reg's subregisters. Consider the following:
+ /// Reg:sub0<def,read-undef> = ...
----------------
This needs an update a well.
================
Comment at: lib/CodeGen/LiveRangeCalc.h:208-209
@@ -164,1 +207,4 @@
+ void computeSubRangeUndefs(SmallVectorImpl<SlotIndex> &Undefs,
+ LiveInterval &LI, LaneBitmask LaneMask) const;
+
----------------
Needs docu.
```
For a given lane compute indexes at which the lane is marked undefined by subregister (def-read-undef) definitions.
```
================
Comment at: lib/CodeGen/RegisterCoalescer.cpp:978
@@ -977,2 +977,3 @@
DefMO.setSubReg(0);
+ DefMO.setIsUndef(false); // Only subregs can have def+undef.
}
----------------
This should go in an independent change (but this line LGTM so no need for an extra review).
================
Comment at: lib/CodeGen/RegisterCoalescer.cpp:2514-2521
@@ +2513,10 @@
+
+ auto IsDefInSubRange = [&LI] (SlotIndex Def) -> bool {
+ for (LiveInterval::SubRange &SR : LI.subranges()) {
+ if (VNInfo *VNI = SR.Query(Def).valueOutOrDead())
+ if (VNI->def == Def)
+ return true;
+ }
+ return false;
+ };
+
----------------
Put this into a static toplevel function instead of a lambda. (This is the common llvm style, uses slightly more familiar syntax and I know how to set set a debugger breakpoint on a function).
================
Comment at: lib/CodeGen/RegisterCoalescer.cpp:2780-2781
@@ -2681,2 +2779,4 @@
}
+ // The merging process may cause the undef information to become invalid
+ // and fail verification. Clear it here, it will be recalculated later.
DEBUG(dbgs() << "\t\tLHST = " << PrintReg(CP.getDstReg())
----------------
remove stale comment.
================
Comment at: lib/CodeGen/SplitKit.cpp:1085-1094
@@ -1029,16 +1084,12 @@
// Extend live ranges to be live-out for successor PHI values.
- for (const VNInfo *PHIVNI : Edit->getParent().valnos) {
- if (PHIVNI->isUnused() || !PHIVNI->isPHIDef())
- continue;
- unsigned RegIdx = RegAssign.lookup(PHIVNI->def);
- LiveRange &LR = LIS.getInterval(Edit->get(RegIdx));
-
- // Check whether PHI is dead.
- const LiveRange::Segment *Segment = LR.getSegmentContaining(PHIVNI->def);
- assert(Segment != nullptr && "Missing segment for VNI");
- if (Segment->end == PHIVNI->def.getDeadSlot()) {
- // This is a dead PHI. Remove it.
- LR.removeSegment(*Segment, true);
- continue;
- }
+ auto RemoveDeadSegment = [] (SlotIndex Def, LiveRange &LR) -> bool {
+ const LiveRange::Segment *Seg = LR.getSegmentContaining(Def);
+ if (Seg == nullptr)
+ return true;
+ if (Seg->end != Def.getDeadSlot())
+ return false;
+ // This is a dead PHI. Remove it.
+ LR.removeSegment(*Seg, true);
+ return true;
+ };
----------------
Pull this out into a static toplevel function.
================
Comment at: lib/CodeGen/SplitKit.cpp:1096-1107
@@ -1044,14 +1095,14 @@
- LiveRangeCalc &LRC = getLRCalc(RegIdx);
- MachineBasicBlock *MBB = LIS.getMBBFromIndex(PHIVNI->def);
- for (MachineBasicBlock::pred_iterator PI = MBB->pred_begin(),
- PE = MBB->pred_end(); PI != PE; ++PI) {
- SlotIndex End = LIS.getMBBEndIdx(*PI);
+ auto ExtendPHIRange = [this] (MachineBasicBlock &B, LiveRangeCalc &LRC,
+ LiveRange &LR,
+ ArrayRef<SlotIndex> Undefs) -> void {
+ for (MachineBasicBlock *P : B.predecessors()) {
+ SlotIndex End = LIS.getMBBEndIdx(P);
SlotIndex LastUse = End.getPrevSlot();
// The predecessor may not have a live-out value. That is OK, like an
// undef PHI operand.
- if (Edit->getParent().liveAt(LastUse)) {
- assert(RegAssign.lookup(LastUse) == RegIdx &&
- "Different register assignment in phi predecessor");
- LRC.extend(LR, End);
- }
+ if (Edit->getParent().liveAt(LastUse))
+ LRC.extend(LR, End, 0, Undefs);
+ }
+ };
+
----------------
dito.
================
Comment at: lib/CodeGen/SplitKit.cpp:1159-1164
@@ +1158,8 @@
+
+ auto getMaskForOp = [this] (const MachineOperand &Op) -> LaneBitmask {
+ unsigned Reg = Op.getReg(), Sub = Op.getSubReg();
+ LaneBitmask LM = Sub != 0 ? TRI.getSubRegIndexLaneMask(Sub)
+ : MRI.getMaxLaneMaskForVReg(Reg);
+ return LM;
+ };
+
----------------
this is only used once and could be inlined.
================
Comment at: lib/CodeGen/SplitKit.h:328-331
@@ -327,1 +327,6 @@
+ /// Find a subrange corresponding to the lane mask LM in live interval LI.
+ /// The interval LI is assumed to contain such a subrange. This function
+ /// is used to find corresponding subranges between the original interval
+ /// and the new intervals.
+ LiveInterval::SubRange &getSubRangeForMask(LaneBitmask LM, LiveInterval &LI);
----------------
Should prefix argument names with `\p` (or `@p`)
================
Comment at: lib/CodeGen/TargetInstrInfo.cpp:381-385
@@ -380,3 +380,7 @@
MachineInstr *MI = MBB.getParent()->CloneMachineInstr(&Orig);
- MI->substituteRegister(MI->getOperand(0).getReg(), DestReg, SubIdx, TRI);
+ MachineOperand &DefOp = MI->getOperand(0);
+ assert(DefOp.isReg() && DefOp.isDef());
+ MI->substituteRegister(DefOp.getReg(), DestReg, SubIdx, TRI);
+ if (!DefOp.getSubReg())
+ DefOp.setIsUndef(false);
MBB.insert(I, MI);
----------------
This will fail if there is more than 1 definition or implicit definitions. It would probably be better to patch MachineOperand::substVirtReg() which in turn is used by MachineInstr::substituteRegister()
================
Comment at: lib/Target/Hexagon/HexagonExpandCondsets.cpp:465
@@ +464,3 @@
+ SlotIndexes &Indexes = *LIS->getSlotIndexes();
+ unsigned VM = MRI->getMaxLaneMaskForVReg(Reg);
+ assert((VM & LM) != 0);
----------------
Should use `LaneBitmask`
================
Comment at: lib/Target/Hexagon/HexagonExpandCondsets.cpp:466-480
@@ +465,17 @@
+ unsigned VM = MRI->getMaxLaneMaskForVReg(Reg);
+ assert((VM & LM) != 0);
+ for (const MachineOperand &MO : MRI->def_operands(Reg)) {
+ if (!MO.isUndef())
+ continue;
+ unsigned SubReg = MO.getSubReg();
+ assert(SubReg != 0 && "Undef should only be set on subreg defs");
+ LaneBitmask M = TRI->getSubRegIndexLaneMask(SubReg);
+ LaneBitmask UM = VM & ~M;
+ if ((UM & LM) != 0) {
+ const MachineInstr &MI = *MO.getParent();
+ bool EarlyClobber = MO.isEarlyClobber();
+ SlotIndex Pos = Indexes.getInstructionIndex(MI).getRegSlot(EarlyClobber);
+ Undefs.push_back(Pos);
+ }
+ }
+
----------------
This block appears to be the same as LiveRangeCalc::computeSubRangeUndefs(). We should find a way to share this code. Like adding a static variant of the function to LiveRangeCalc so you can call it independently of constructing a LiveRangeCalc instance.
================
Comment at: test/CodeGen/Hexagon/regalloc-bad-undef.mir:68-71
@@ +67,6 @@
+alignment: 2
+exposesReturnsTwice: false
+hasInlineAsm: false
+allVRegsAllocated: false
+isSSA: false
+tracksRegLiveness: true
----------------
test can be simplified by leaving the xxx: false things out as that is the default anyway.
Repository:
rL LLVM
https://reviews.llvm.org/D21189
More information about the llvm-commits
mailing list