Skip to content

Commit 1b38a71

Browse files
cfallincfallin
authored
Some fixes to allow for call instructions to name args, returns, and clobbers with constraints. (#74)
* Some fixes to allow for call instructions to name args, returns, and clobbers with constraints. - Allow early-pos uses with fixed regs that conflict with clobbers (which happen at late-pos), in addition to the existing logic for conflicts with late-pos defs with fixed regs. This is a pretty subtle issue that was uncovered in #53 for the def case, and the fix here is the mirror of that fix for clobbers. The root cause for all this complexity is that we can't split in the middle of an instruction (because there's no way to insert a move there!) so if a use is live-downward, we can't let it live in preg A at early-pos and preg B != A at late-pos; instead we need to rewrite the constraints and use a fixup move. The earlier change to fix #53 was actually a bit too conservative in that it always applied when such conflicts existed, even if the downward arg was not live. This PR fixes that (it's fine for the early-use and late-def to be fixed to the same reg if the use's liverange ends after early-pos) and adapts the same flexibility to the clobbers case as well. - Reworks the fixups for the def case mentioned above to not shift the def to the Early point. Doing so causes issues when the def is to a reffy vreg: it can then be falsely included in a stackmap if the instruction containing this operand is a safepoint. - Fixes the last-resort split-bundle-into-minimal-pieces logic from #59 to properly limit a minimal bundle piece to end after the early-pos, rather than cover the entire instruction. This was causing artificial overlaps between args that end after early-pos and defs that start at late-pos when one of the vregs hit the fallback split behavior. * Fix fuzzbug: do not merge when a liverange has a fixed-reg def. This can create impossible situations: e.g., if a vreg is constrained to p0 as a late-def, and another, completely different vreg is constrained to p0 as an early-use on the same instruction, and the instruction also has a third vreg (early-use), we do not want to merge the liverange for the third vreg with the first, because it would result in an unsolveable conflict for p0 at the early-point. * Review comments.
1 parent 906a053 commit 1b38a71

6 files changed

Lines changed: 100 additions & 48 deletions

File tree

‎src/ion/data_structures.rs‎

Lines changed: 25 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -53,6 +53,14 @@ impl CodeRange {
5353
pub fn len(&self) -> usize {
5454
self.to.inst().index() - self.from.inst().index()
5555
}
56+
/// Returns the range covering just one program point.
57+
#[inline(always)]
58+
pub fn singleton(pos: ProgPoint) -> CodeRange {
59+
CodeRange {
60+
from: pos,
61+
to: pos.next(),
62+
}
63+
}
5664
}
5765

5866
impl std::cmp::PartialOrd for CodeRange {
@@ -185,7 +193,7 @@ pub struct LiveBundle {
185193
pub spill_weight_and_props: u32,
186194
}
187195

188-
pub const BUNDLE_MAX_SPILL_WEIGHT: u32 = (1 << 29) - 1;
196+
pub const BUNDLE_MAX_SPILL_WEIGHT: u32 = (1 << 28) - 1;
189197
pub const MINIMAL_FIXED_BUNDLE_SPILL_WEIGHT: u32 = BUNDLE_MAX_SPILL_WEIGHT;
190198
pub const MINIMAL_BUNDLE_SPILL_WEIGHT: u32 = BUNDLE_MAX_SPILL_WEIGHT - 1;
191199
pub const BUNDLE_MAX_NORMAL_SPILL_WEIGHT: u32 = BUNDLE_MAX_SPILL_WEIGHT - 2;
@@ -197,13 +205,15 @@ impl LiveBundle {
197205
spill_weight: u32,
198206
minimal: bool,
199207
fixed: bool,
208+
fixed_def: bool,
200209
stack: bool,
201210
) {
202211
debug_assert!(spill_weight <= BUNDLE_MAX_SPILL_WEIGHT);
203212
self.spill_weight_and_props = spill_weight
204213
| (if minimal { 1 << 31 } else { 0 })
205214
| (if fixed { 1 << 30 } else { 0 })
206-
| (if stack { 1 << 29 } else { 0 });
215+
| (if fixed_def { 1 << 29 } else { 0 })
216+
| (if stack { 1 << 28 } else { 0 });
207217
}
208218

209219
#[inline(always)]
@@ -217,23 +227,33 @@ impl LiveBundle {
217227
}
218228

219229
#[inline(always)]
220-
pub fn cached_stack(&self) -> bool {
230+
pub fn cached_fixed_def(&self) -> bool {
221231
self.spill_weight_and_props & (1 << 29) != 0
222232
}
223233

234+
#[inline(always)]
235+
pub fn cached_stack(&self) -> bool {
236+
self.spill_weight_and_props & (1 << 28) != 0
237+
}
238+
224239
#[inline(always)]
225240
pub fn set_cached_fixed(&mut self) {
226241
self.spill_weight_and_props |= 1 << 30;
227242
}
228243

229244
#[inline(always)]
230-
pub fn set_cached_stack(&mut self) {
245+
pub fn set_cached_fixed_def(&mut self) {
231246
self.spill_weight_and_props |= 1 << 29;
232247
}
233248

249+
#[inline(always)]
250+
pub fn set_cached_stack(&mut self) {
251+
self.spill_weight_and_props |= 1 << 28;
252+
}
253+
234254
#[inline(always)]
235255
pub fn cached_spill_weight(&self) -> u32 {
236-
self.spill_weight_and_props & ((1 << 29) - 1)
256+
self.spill_weight_and_props & ((1 << 28) - 1)
237257
}
238258
}
239259

‎src/ion/liveranges.rs‎

Lines changed: 34 additions & 30 deletions
Original file line numberDiff line numberDiff line change
@@ -935,11 +935,8 @@ impl<'a, F: Function> Env<'a, F> {
935935
// constraint, and (ii) move the def to Early position
936936
// to reserve the register for the whole instruction.
937937
let mut operand_rewrites: FxHashMap<usize, Operand> = FxHashMap::default();
938-
let mut late_def_fixed: SmallVec<[(PReg, Operand, usize); 2]> = smallvec![];
939-
for (i, &operand) in self.func.inst_operands(inst).iter().enumerate() {
940-
if operand.as_fixed_nonallocatable().is_some() {
941-
continue;
942-
}
938+
let mut late_def_fixed: SmallVec<[PReg; 8]> = smallvec![];
939+
for &operand in self.func.inst_operands(inst) {
943940
if let OperandConstraint::FixedReg(preg) = operand.constraint() {
944941
match operand.pos() {
945942
OperandPos::Late => {
@@ -954,7 +951,7 @@ impl<'a, F: Function> Env<'a, F> {
954951
"Invalid operand: fixed constraint on Use/Mod at Late point"
955952
);
956953

957-
late_def_fixed.push((preg, operand, i));
954+
late_def_fixed.push(preg);
958955
}
959956
_ => {}
960957
}
@@ -966,19 +963,19 @@ impl<'a, F: Function> Env<'a, F> {
966963
}
967964
if let OperandConstraint::FixedReg(preg) = operand.constraint() {
968965
match operand.pos() {
969-
OperandPos::Early => {
966+
OperandPos::Early if live.get(operand.vreg().vreg()) => {
970967
assert!(operand.kind() == OperandKind::Use,
971968
"Invalid operand: fixed constraint on Def/Mod at Early position");
972969

973970
// If we have a constraint at the
974971
// Early point for a fixed preg, and
975972
// this preg is also constrained with
976-
// a *separate* def at Late, and *if*
977-
// the vreg is live downward, we have
978-
// to use the multi-fixed-reg
979-
// mechanism for a fixup and rewrite
980-
// here without the constraint. See
981-
// #53.
973+
// a *separate* def at Late or is
974+
// clobbered, and *if* the vreg is
975+
// live downward, we have to use the
976+
// multi-fixed-reg mechanism for a
977+
// fixup and rewrite here without the
978+
// constraint. See #53.
982979
//
983980
// We adjust the def liverange and Use
984981
// to an "early" position to reserve
@@ -990,10 +987,18 @@ impl<'a, F: Function> Env<'a, F> {
990987
// conflicting constraints for the
991988
// same vreg in a separate pass (see
992989
// `fixup_multi_fixed_vregs` below).
993-
if let Some((_, def_op, def_slot)) = late_def_fixed
994-
.iter()
995-
.find(|(def_preg, _, _)| *def_preg == preg)
990+
if late_def_fixed.contains(&preg)
991+
|| self.func.inst_clobbers(inst).contains(preg)
996992
{
993+
log::trace!(
994+
concat!(
995+
"-> operand {:?} is fixed to preg {:?}, ",
996+
"is downward live, and there is also a ",
997+
"def or clobber at this preg"
998+
),
999+
operand,
1000+
preg
1001+
);
9971002
let pos = ProgPoint::before(inst);
9981003
self.multi_fixed_reg_fixups.push(MultiFixedRegFixup {
9991004
pos,
@@ -1004,6 +1009,14 @@ impl<'a, F: Function> Env<'a, F> {
10041009
level: FixedRegFixupLevel::Initial,
10051010
});
10061011

1012+
// We need to insert a reservation
1013+
// at the before-point to reserve
1014+
// the reg for the use too.
1015+
let range = CodeRange::singleton(pos);
1016+
self.add_liverange_to_preg(range, preg);
1017+
1018+
// Remove the fixed-preg
1019+
// constraint from the Use.
10071020
operand_rewrites.insert(
10081021
i,
10091022
Operand::new(
@@ -1013,15 +1026,6 @@ impl<'a, F: Function> Env<'a, F> {
10131026
operand.pos(),
10141027
),
10151028
);
1016-
operand_rewrites.insert(
1017-
*def_slot,
1018-
Operand::new(
1019-
def_op.vreg(),
1020-
def_op.constraint(),
1021-
def_op.kind(),
1022-
OperandPos::Early,
1023-
),
1024-
);
10251029
}
10261030
}
10271031
_ => {}
@@ -1310,7 +1314,7 @@ impl<'a, F: Function> Env<'a, F> {
13101314
// have to split the multiple uses at the same progpoint into
13111315
// different bundles, which breaks invariants related to
13121316
// disjoint ranges and bundles).
1313-
let mut extra_clobbers: SmallVec<[(PReg, Inst); 8]> = smallvec![];
1317+
let mut extra_clobbers: SmallVec<[(PReg, ProgPoint); 8]> = smallvec![];
13141318
for vreg in 0..self.vregs.len() {
13151319
for range_idx in 0..self.vregs[vreg].ranges.len() {
13161320
let entry = self.vregs[vreg].ranges[range_idx];
@@ -1419,15 +1423,15 @@ impl<'a, F: Function> Env<'a, F> {
14191423
u.operand.pos(),
14201424
);
14211425
trace!(" -> extra clobber {} at inst{}", preg, u.pos.inst().index());
1422-
extra_clobbers.push((preg, u.pos.inst()));
1426+
extra_clobbers.push((preg, u.pos));
14231427
}
14241428
}
14251429
}
14261430

1427-
for &(clobber, inst) in &extra_clobbers {
1431+
for &(clobber, pos) in &extra_clobbers {
14281432
let range = CodeRange {
1429-
from: ProgPoint::before(inst),
1430-
to: ProgPoint::before(inst.next()),
1433+
from: pos,
1434+
to: pos.next(),
14311435
};
14321436
self.add_liverange_to_preg(range, clobber);
14331437
}

‎src/ion/merge.rs‎

Lines changed: 22 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -16,7 +16,9 @@ use super::{
1616
Env, LiveBundleIndex, LiveRangeIndex, LiveRangeKey, SpillSet, SpillSetIndex, SpillSlotIndex,
1717
VRegIndex,
1818
};
19-
use crate::{ion::data_structures::BlockparamOut, Function, Inst, OperandConstraint, PReg};
19+
use crate::{
20+
ion::data_structures::BlockparamOut, Function, Inst, OperandConstraint, OperandKind, PReg,
21+
};
2022
use smallvec::smallvec;
2123

2224
impl<'a, F: Function> Env<'a, F> {
@@ -93,6 +95,17 @@ impl<'a, F: Function> Env<'a, F> {
9395
}
9496
}
9597

98+
// Avoid merging if either side has a fixed-reg def: this can
99+
// result in an impossible-to-solve allocation problem if
100+
// there is a fixed-reg use in the same reg on the same
101+
// instruction.
102+
if self.bundles[from.index()].cached_fixed_def()
103+
|| self.bundles[to.index()].cached_fixed_def()
104+
{
105+
trace!(" -> one bundle has a fixed def; aborting merge");
106+
return false;
107+
}
108+
96109
// Check for a requirements conflict.
97110
if self.bundles[from.index()].cached_stack()
98111
|| self.bundles[from.index()].cached_fixed()
@@ -258,23 +271,30 @@ impl<'a, F: Function> Env<'a, F> {
258271
}
259272

260273
let mut fixed = false;
274+
let mut fixed_def = false;
261275
let mut stack = false;
262276
for entry in &self.bundles[bundle.index()].ranges {
263277
for u in &self.ranges[entry.index.index()].uses {
264278
if let OperandConstraint::FixedReg(_) = u.operand.constraint() {
265279
fixed = true;
280+
if u.operand.kind() == OperandKind::Def {
281+
fixed_def = true;
282+
}
266283
}
267284
if let OperandConstraint::Stack = u.operand.constraint() {
268285
stack = true;
269286
}
270-
if fixed && stack {
287+
if fixed && stack && fixed_def {
271288
break;
272289
}
273290
}
274291
}
275292
if fixed {
276293
self.bundles[bundle.index()].set_cached_fixed();
277294
}
295+
if fixed_def {
296+
self.bundles[bundle.index()].set_cached_fixed_def();
297+
}
278298
if stack {
279299
self.bundles[bundle.index()].set_cached_stack();
280300
}

‎src/ion/process.rs‎

Lines changed: 17 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -262,6 +262,7 @@ impl<'a, F: Function> Env<'a, F> {
262262

263263
let minimal;
264264
let mut fixed = false;
265+
let mut fixed_def = false;
265266
let mut stack = false;
266267
let bundledata = &self.bundles[bundle.index()];
267268
let first_range = bundledata.ranges[0].index;
@@ -277,11 +278,15 @@ impl<'a, F: Function> Env<'a, F> {
277278
for u in &first_range_data.uses {
278279
trace!(" -> use: {:?}", u);
279280
if let OperandConstraint::FixedReg(_) = u.operand.constraint() {
280-
trace!(" -> fixed use at {:?}: {:?}", u.pos, u.operand);
281+
trace!(" -> fixed operand at {:?}: {:?}", u.pos, u.operand);
281282
fixed = true;
283+
if u.operand.kind() == OperandKind::Def {
284+
trace!(" -> is fixed def");
285+
fixed_def = true;
286+
}
282287
}
283288
if let OperandConstraint::Stack = u.operand.constraint() {
284-
trace!(" -> stack use at {:?}: {:?}", u.pos, u.operand);
289+
trace!(" -> stack operand at {:?}: {:?}", u.pos, u.operand);
285290
stack = true;
286291
}
287292
if stack && fixed {
@@ -340,6 +345,7 @@ impl<'a, F: Function> Env<'a, F> {
340345
spill_weight,
341346
minimal,
342347
fixed,
348+
fixed_def,
343349
stack,
344350
);
345351
}
@@ -794,6 +800,7 @@ impl<'a, F: Function> Env<'a, F> {
794800
for entry_idx in 0..self.bundles[bundle.index()].ranges.len() {
795801
// Iterate manually; don't borrow `self`.
796802
let entry = self.bundles[bundle.index()].ranges[entry_idx];
803+
let lr_to = entry.range.to;
797804

798805
removed_lrs.insert(entry.index);
799806
let vreg = self.ranges[entry.index.index()].vreg;
@@ -813,14 +820,17 @@ impl<'a, F: Function> Env<'a, F> {
813820
continue;
814821
}
815822

823+
// The minimal bundle runs through the whole inst
824+
// (up to the Before of the next inst), *unless*
825+
// the original LR was only over the Before (up to
826+
// the After) of this inst.
827+
let to = std::cmp::min(ProgPoint::before(u.pos.inst().next()), lr_to);
828+
816829
// If the last bundle was at the same inst, add a new
817830
// LR to the same bundle; otherwise, create a LR and a
818831
// new bundle.
819832
if Some(u.pos.inst()) == last_inst {
820-
let cr = CodeRange {
821-
from: u.pos,
822-
to: ProgPoint::before(u.pos.inst().next()),
823-
};
833+
let cr = CodeRange { from: u.pos, to };
824834
let lr = self.create_liverange(cr);
825835
new_lrs.push((vreg, lr));
826836
self.ranges[lr.index()].uses.push(u);
@@ -853,10 +863,7 @@ impl<'a, F: Function> Env<'a, F> {
853863

854864
// Otherwise, create a new LR.
855865
let pos = ProgPoint::before(u.pos.inst());
856-
let cr = CodeRange {
857-
from: pos,
858-
to: ProgPoint::before(u.pos.inst().next()),
859-
};
866+
let cr = CodeRange { from: pos, to };
860867
let lr = self.create_liverange(cr);
861868
new_lrs.push((vreg, lr));
862869
self.ranges[lr.index()].uses.push(u);

‎src/ion/requirement.rs‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -130,7 +130,7 @@ impl<'a, F: Function> Env<'a, F> {
130130
trace!("compute_requirement: {:?}", bundle);
131131
let ranges = &self.bundles[bundle.index()].ranges;
132132
for entry in ranges {
133-
trace!(" -> LR {:?}", entry.index);
133+
trace!(" -> LR {:?}: {:?}", entry.index, entry.range);
134134
for u in &self.ranges[entry.index.index()].uses {
135135
trace!(" -> use {:?}", u);
136136
let r = self.requirement_from_operand(u.operand);

‎src/ion/stackmap.rs‎

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -55,6 +55,7 @@ impl<'a, F: Function> Env<'a, F> {
5555
safepoint_idx += 1;
5656
continue;
5757
}
58+
5859
trace!(" -> covers safepoint {:?}", safepoints[safepoint_idx]);
5960

6061
self.safepoint_slots

0 commit comments

Comments
 (0)