Skip to content

Some fixes to allow for call instructions to name args, returns, and clobbers with constraints. #74

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
cfallin merged 3 commits into from
Sep 20, 2022
Merged

Some fixes to allow for call instructions to name args, returns, and clobbers with constraints. #74

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
de30447
Some fixes to allow for call instructions to name args, returns, and …
cfallin Sep 9, 2022
98adab7
Fix fuzzbug: do not merge when a liverange has a fixed-reg def.
cfallin Sep 15, 2022
ac8e5f4
Review comments.
cfallin Sep 20, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
30 changes: 25 additions & 5 deletions src/ion/data_structures.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -53,6 +53,14 @@ impl CodeRange {
pub fn len(&self) -> usize {
self.to.inst().index() - self.from.inst().index()
}
/// Returns the range covering just one program point.
#[inline(always)]
pub fn singleton(pos: ProgPoint) -> CodeRange {
CodeRange {
from: pos,
to: pos.next(),
}
}
}

impl std::cmp::PartialOrd for CodeRange {
Expand Down Expand Up @@ -185,7 +193,7 @@ pub struct LiveBundle {
pub spill_weight_and_props: u32,
}

pub const BUNDLE_MAX_SPILL_WEIGHT: u32 = (1 << 29) - 1;
pub const BUNDLE_MAX_SPILL_WEIGHT: u32 = (1 << 28) - 1;
pub const MINIMAL_FIXED_BUNDLE_SPILL_WEIGHT: u32 = BUNDLE_MAX_SPILL_WEIGHT;
pub const MINIMAL_BUNDLE_SPILL_WEIGHT: u32 = BUNDLE_MAX_SPILL_WEIGHT - 1;
pub const BUNDLE_MAX_NORMAL_SPILL_WEIGHT: u32 = BUNDLE_MAX_SPILL_WEIGHT - 2;
Expand All @@ -197,13 +205,15 @@ impl LiveBundle {
spill_weight: u32,
minimal: bool,
fixed: bool,
fixed_def: bool,
stack: bool,
) {
debug_assert!(spill_weight <= BUNDLE_MAX_SPILL_WEIGHT);
self.spill_weight_and_props = spill_weight
| (if minimal { 1 << 31 } else { 0 })
| (if fixed { 1 << 30 } else { 0 })
| (if stack { 1 << 29 } else { 0 });
| (if fixed_def { 1 << 29 } else { 0 })
| (if stack { 1 << 28 } else { 0 });
}

#[inline(always)]
Expand All @@ -217,23 +227,33 @@ impl LiveBundle {
}

#[inline(always)]
pub fn cached_stack(&self) -> bool {
pub fn cached_fixed_def(&self) -> bool {
self.spill_weight_and_props & (1 << 29) != 0
}

#[inline(always)]
pub fn cached_stack(&self) -> bool {
self.spill_weight_and_props & (1 << 28) != 0
}

#[inline(always)]
pub fn set_cached_fixed(&mut self) {
self.spill_weight_and_props |= 1 << 30;
}

#[inline(always)]
pub fn set_cached_stack(&mut self) {
pub fn set_cached_fixed_def(&mut self) {
self.spill_weight_and_props |= 1 << 29;
}

#[inline(always)]
pub fn set_cached_stack(&mut self) {
self.spill_weight_and_props |= 1 << 28;
}

#[inline(always)]
pub fn cached_spill_weight(&self) -> u32 {
self.spill_weight_and_props & ((1 << 29) - 1)
self.spill_weight_and_props & ((1 << 28) - 1)
}
}

Expand Down
64 changes: 34 additions & 30 deletions src/ion/liveranges.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -935,11 +935,8 @@ impl<'a, F: Function> Env<'a, F> {
// constraint, and (ii) move the def to Early position
// to reserve the register for the whole instruction.
let mut operand_rewrites: FxHashMap<usize, Operand> = FxHashMap::default();
let mut late_def_fixed: SmallVec<[(PReg, Operand, usize); 2]> = smallvec![];
for (i, &operand) in self.func.inst_operands(inst).iter().enumerate() {
if operand.as_fixed_nonallocatable().is_some() {
continue;
}
let mut late_def_fixed: SmallVec<[PReg; 8]> = smallvec![];
for &operand in self.func.inst_operands(inst) {
if let OperandConstraint::FixedReg(preg) = operand.constraint() {
match operand.pos() {
OperandPos::Late => {
Expand All @@ -954,7 +951,7 @@ impl<'a, F: Function> Env<'a, F> {
"Invalid operand: fixed constraint on Use/Mod at Late point"
);

late_def_fixed.push((preg, operand, i));
late_def_fixed.push(preg);
}
_ => {}
}
Expand All @@ -966,19 +963,19 @@ impl<'a, F: Function> Env<'a, F> {
}
if let OperandConstraint::FixedReg(preg) = operand.constraint() {
match operand.pos() {
OperandPos::Early => {
OperandPos::Early if live.get(operand.vreg().vreg()) => {
assert!(operand.kind() == OperandKind::Use,
"Invalid operand: fixed constraint on Def/Mod at Early position");

// If we have a constraint at the
// Early point for a fixed preg, and
// this preg is also constrained with
// a *separate* def at Late, and *if*
// the vreg is live downward, we have
// to use the multi-fixed-reg
// mechanism for a fixup and rewrite
// here without the constraint. See
// #53.
// a *separate* def at Late or is
// clobbered, and *if* the vreg is
// live downward, we have to use the
// multi-fixed-reg mechanism for a
// fixup and rewrite here without the
// constraint. See #53.
//
// We adjust the def liverange and Use
// to an "early" position to reserve
Expand All @@ -990,10 +987,18 @@ impl<'a, F: Function> Env<'a, F> {
// conflicting constraints for the
// same vreg in a separate pass (see
// `fixup_multi_fixed_vregs` below).
if let Some((_, def_op, def_slot)) = late_def_fixed
.iter()
.find(|(def_preg, _, _)| *def_preg == preg)
if late_def_fixed.contains(&preg)
|| self.func.inst_clobbers(inst).contains(preg)
{
log::trace!(
concat!(
"-> operand {:?} is fixed to preg {:?}, ",
"is downward live, and there is also a ",
"def or clobber at this preg"
),
operand,
preg
);
let pos = ProgPoint::before(inst);
self.multi_fixed_reg_fixups.push(MultiFixedRegFixup {
pos,
Expand All @@ -1004,6 +1009,14 @@ impl<'a, F: Function> Env<'a, F> {
level: FixedRegFixupLevel::Initial,
});

// We need to insert a reservation
// at the before-point to reserve
// the reg for the use too.
let range = CodeRange::singleton(pos);
self.add_liverange_to_preg(range, preg);

// Remove the fixed-preg
// constraint from the Use.
operand_rewrites.insert(
i,
Operand::new(
Expand All @@ -1013,15 +1026,6 @@ impl<'a, F: Function> Env<'a, F> {
operand.pos(),
),
);
operand_rewrites.insert(
*def_slot,
Operand::new(
def_op.vreg(),
def_op.constraint(),
def_op.kind(),
OperandPos::Early,
),
);
}
}
_ => {}
Expand Down Expand Up @@ -1310,7 +1314,7 @@ impl<'a, F: Function> Env<'a, F> {
// have to split the multiple uses at the same progpoint into
// different bundles, which breaks invariants related to
// disjoint ranges and bundles).
let mut extra_clobbers: SmallVec<[(PReg, Inst); 8]> = smallvec![];
let mut extra_clobbers: SmallVec<[(PReg, ProgPoint); 8]> = smallvec![];
for vreg in 0..self.vregs.len() {
for range_idx in 0..self.vregs[vreg].ranges.len() {
let entry = self.vregs[vreg].ranges[range_idx];
Expand Down Expand Up @@ -1419,15 +1423,15 @@ impl<'a, F: Function> Env<'a, F> {
u.operand.pos(),
);
trace!(" -> extra clobber {} at inst{}", preg, u.pos.inst().index());
extra_clobbers.push((preg, u.pos.inst()));
extra_clobbers.push((preg, u.pos));
}
}
}

for &(clobber, inst) in &extra_clobbers {
for &(clobber, pos) in &extra_clobbers {
let range = CodeRange {
from: ProgPoint::before(inst),
to: ProgPoint::before(inst.next()),
from: pos,
to: pos.next(),
};
self.add_liverange_to_preg(range, clobber);
}
Expand Down
24 changes: 22 additions & 2 deletions src/ion/merge.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,7 +16,9 @@ use super::{
Env, LiveBundleIndex, LiveRangeIndex, LiveRangeKey, SpillSet, SpillSetIndex, SpillSlotIndex,
VRegIndex,
};
use crate::{ion::data_structures::BlockparamOut, Function, Inst, OperandConstraint, PReg};
use crate::{
ion::data_structures::BlockparamOut, Function, Inst, OperandConstraint, OperandKind, PReg,
};
use smallvec::smallvec;

impl<'a, F: Function> Env<'a, F> {
Expand Down Expand Up @@ -93,6 +95,17 @@ impl<'a, F: Function> Env<'a, F> {
}
}

// Avoid merging if either side has a fixed-reg def: this can
// result in an impossible-to-solve allocation problem if
// there is a fixed-reg use in the same reg on the same
// instruction.
if self.bundles[from.index()].cached_fixed_def()
|| self.bundles[to.index()].cached_fixed_def()
Comment on lines +102 to +103
Copy link
Copy Markdown
Member

@elliottt elliottt Sep 20, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If you knew that the fixed def constraints were for the same register, would that change whether or not you needed to bail out here?

Copy link
Copy Markdown
Member Author

@cfallin cfallin Sep 20, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No, in fact the fuzzbug was exactly this case! The issue is that the way that we do the constraint rewrite adds an anonymous reservation; so this isn't a case of "different fixed def, can't merge" (that is captured by the incompatible-requirement check) but rather "corner case results in impossible problem".

The interesting case is when we have a fixed-use bound to preg P with liverange up to the early point on an inst, a fixed-def bound to preg P (same preg) at the late point on an inst, and that def'd vreg flows into a blockparam arg and over a backedge to the use's vreg. Then we try to merge the two vregs into one bundle (because we try this for every blockparam arg - blockparam def pair). But we rewrote the use to be an "any" use and put an anonymous reservation on preg P at the early-point only. So rejoining the first vreg (the use-side) to the whole bundle and thus requiring it to have the fixed-preg constraint again causes an impossible-to-solve problem.

There may be an even more complex way to rewrite this but I was several levels deep into "introduce fix, get deeper bug" and wanted to bail out of the rabbithole altogether...

Copy link
Copy Markdown
Contributor

@Amanieu Amanieu Jul 12, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This check is very conservative and is hurting our compiler quite a bit since it makes heavy use of fixed operands. Removing this check gives a 4% code size reduction (I enabled the checker to ensure the result is still valid).

What are the exact requirements that cause the impossible to resolve constraint? Could this check be made more specific?

Copy link
Copy Markdown
Member Author

@cfallin cfallin Jul 12, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't really have details paged in anymore, beyond the paragraph describing a fuzzing-discovered example I wrote above, nor the cycles at the moment to think through it deeply. You're welcome to remove it and fuzz to (re)discover it, and propose a better fix, of course!

{
trace!(" -> one bundle has a fixed def; aborting merge");
return false;
}

// Check for a requirements conflict.
if self.bundles[from.index()].cached_stack()
|| self.bundles[from.index()].cached_fixed()
Expand Down Expand Up @@ -258,23 +271,30 @@ impl<'a, F: Function> Env<'a, F> {
}

let mut fixed = false;
let mut fixed_def = false;
let mut stack = false;
for entry in &self.bundles[bundle.index()].ranges {
for u in &self.ranges[entry.index.index()].uses {
if let OperandConstraint::FixedReg(_) = u.operand.constraint() {
fixed = true;
if u.operand.kind() == OperandKind::Def {
fixed_def = true;
}
}
if let OperandConstraint::Stack = u.operand.constraint() {
stack = true;
}
if fixed && stack {
if fixed && stack && fixed_def {
break;
}
}
}
if fixed {
self.bundles[bundle.index()].set_cached_fixed();
}
if fixed_def {
self.bundles[bundle.index()].set_cached_fixed_def();
Copy link
Copy Markdown
Member

@elliottt elliottt Sep 20, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Does this flag ever get cleared, or does it follow the parts of the bundle if it's split up?

Copy link
Copy Markdown
Member Author

@cfallin cfallin Sep 20, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's recomputed along with the other flags when the bundle is split, if it ever is (it actually is a flag on the whole bundle!).

Copy link
Copy Markdown
Member Author

@cfallin cfallin Sep 20, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

https://github.com/cfallin/regalloc2/blob/ac8e5f4e22791e7e554c2c8f2ddea28768bbdc78/src/ion/process.rs#L344-L350

}
if stack {
self.bundles[bundle.index()].set_cached_stack();
}
Expand Down
27 changes: 17 additions & 10 deletions src/ion/process.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -262,6 +262,7 @@ impl<'a, F: Function> Env<'a, F> {

let minimal;
let mut fixed = false;
let mut fixed_def = false;
let mut stack = false;
let bundledata = &self.bundles[bundle.index()];
let first_range = bundledata.ranges[0].index;
Expand All @@ -277,11 +278,15 @@ impl<'a, F: Function> Env<'a, F> {
for u in &first_range_data.uses {
trace!(" -> use: {:?}", u);
if let OperandConstraint::FixedReg(_) = u.operand.constraint() {
trace!(" -> fixed use at {:?}: {:?}", u.pos, u.operand);
trace!(" -> fixed operand at {:?}: {:?}", u.pos, u.operand);
fixed = true;
if u.operand.kind() == OperandKind::Def {
trace!(" -> is fixed def");
fixed_def = true;
}
}
if let OperandConstraint::Stack = u.operand.constraint() {
trace!(" -> stack use at {:?}: {:?}", u.pos, u.operand);
trace!(" -> stack operand at {:?}: {:?}", u.pos, u.operand);
stack = true;
}
if stack && fixed {
Expand Down Expand Up @@ -340,6 +345,7 @@ impl<'a, F: Function> Env<'a, F> {
spill_weight,
minimal,
fixed,
fixed_def,
stack,
);
}
Expand Down Expand Up @@ -794,6 +800,7 @@ impl<'a, F: Function> Env<'a, F> {
for entry_idx in 0..self.bundles[bundle.index()].ranges.len() {
// Iterate manually; don't borrow `self`.
let entry = self.bundles[bundle.index()].ranges[entry_idx];
let lr_to = entry.range.to;

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

// The minimal bundle runs through the whole inst
// (up to the Before of the next inst), *unless*
// the original LR was only over the Before (up to
// the After) of this inst.
let to = std::cmp::min(ProgPoint::before(u.pos.inst().next()), lr_to);

// If the last bundle was at the same inst, add a new
// LR to the same bundle; otherwise, create a LR and a
// new bundle.
if Some(u.pos.inst()) == last_inst {
let cr = CodeRange {
from: u.pos,
to: ProgPoint::before(u.pos.inst().next()),
};
let cr = CodeRange { from: u.pos, to };
let lr = self.create_liverange(cr);
new_lrs.push((vreg, lr));
self.ranges[lr.index()].uses.push(u);
Expand Down Expand Up @@ -853,10 +863,7 @@ impl<'a, F: Function> Env<'a, F> {

// Otherwise, create a new LR.
let pos = ProgPoint::before(u.pos.inst());
let cr = CodeRange {
from: pos,
to: ProgPoint::before(u.pos.inst().next()),
};
let cr = CodeRange { from: pos, to };
let lr = self.create_liverange(cr);
new_lrs.push((vreg, lr));
self.ranges[lr.index()].uses.push(u);
Expand Down
2 changes: 1 addition & 1 deletion src/ion/requirement.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -130,7 +130,7 @@ impl<'a, F: Function> Env<'a, F> {
trace!("compute_requirement: {:?}", bundle);
let ranges = &self.bundles[bundle.index()].ranges;
for entry in ranges {
trace!(" -> LR {:?}", entry.index);
trace!(" -> LR {:?}: {:?}", entry.index, entry.range);
for u in &self.ranges[entry.index.index()].uses {
trace!(" -> use {:?}", u);
let r = self.requirement_from_operand(u.operand);
Expand Down
1 change: 1 addition & 0 deletions src/ion/stackmap.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -55,6 +55,7 @@ impl<'a, F: Function> Env<'a, F> {
safepoint_idx += 1;
continue;
}

trace!(" -> covers safepoint {:?}", safepoints[safepoint_idx]);

self.safepoint_slots
Expand Down