Skip to content
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

ISLE: Allow emitting safepoint insns #3723

Merged
merged 1 commit into from
Jan 25, 2022
Merged

ISLE: Allow emitting safepoint insns #3723

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
11 changes: 7 additions & 4 deletions cranelift/codegen/src/isa/aarch64/lower/isle.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -152,14 +152,17 @@ where
let imm =
MoveWideConst::maybe_with_shift(((!imm16) & 0xffff) as u16, i * 16)
.unwrap();
self.emitted_insts.push(MInst::MovN { rd, imm, size });
self.emitted_insts
.push((MInst::MovN { rd, imm, size }, false));
} else {
let imm = MoveWideConst::maybe_with_shift(imm16 as u16, i * 16).unwrap();
self.emitted_insts.push(MInst::MovZ { rd, imm, size });
self.emitted_insts
.push((MInst::MovZ { rd, imm, size }, false));
}
} else {
let imm = MoveWideConst::maybe_with_shift(imm16 as u16, i * 16).unwrap();
self.emitted_insts.push(MInst::MovK { rd, imm, size });
self.emitted_insts
.push((MInst::MovK { rd, imm, size }, false));
Comment on lines +155 to +165
Copy link
Member

Choose a reason for hiding this comment

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

Could we use self.emit(..) here to hide the bool tuple entry, which is noise/distracting/raises irrelevant questions for new code readers who stumble upon this?

Copy link
Member

Choose a reason for hiding this comment

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

Ah I see this already merged, I'll send a follow up PR.

}
}
}
Expand Down Expand Up @@ -200,7 +203,7 @@ where
}

fn emit(&mut self, inst: &MInst) -> Unit {
self.emitted_insts.push(inst.clone());
self.emitted_insts.push((inst.clone(), false));
}

fn cond_br_zero(&mut self, reg: Reg) -> CondBrKind {
Expand Down
2 changes: 1 addition & 1 deletion cranelift/codegen/src/isa/s390x/lower/isle.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -470,6 +470,6 @@ where

#[inline]
fn emit(&mut self, inst: &MInst) -> Unit {
self.emitted_insts.push(inst.clone());
self.emitted_insts.push((inst.clone(), false));
}
}
2 changes: 1 addition & 1 deletion cranelift/codegen/src/isa/x64/lower/isle.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -227,7 +227,7 @@ where

fn emit(&mut self, inst: &MInst) -> Unit {
for inst in inst.clone().mov_mitosis() {
self.emitted_insts.push(inst);
self.emitted_insts.push((inst, false));
}
}

Expand Down
16 changes: 10 additions & 6 deletions cranelift/codegen/src/machinst/isle.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -299,12 +299,12 @@ macro_rules! isle_prelude_methods {
/// internally has a temporary reference to a machinst `LowerCtx`.
pub(crate) struct IsleContext<'a, C: LowerCtx, F, I, const N: usize>
where
[C::I; N]: smallvec::Array,
[(C::I, bool); N]: smallvec::Array,
{
pub lower_ctx: &'a mut C,
pub flags: &'a F,
pub isa_flags: &'a I,
pub emitted_insts: SmallVec<[C::I; N]>,
pub emitted_insts: SmallVec<[(C::I, bool); N]>,
Copy link
Contributor

Choose a reason for hiding this comment

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

This probably increases memory usage by a considerable amount due to padding. Maybe we could instead have an is_safepoint method on instructions?

Copy link
Member Author

Choose a reason for hiding this comment

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

This vector tends to be small, it only holds the machine instructions emitted for a single CLIF instruction. So I don't think memory usage is really any concern here ...

Copy link
Contributor

Choose a reason for hiding this comment

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

Right, forgot about that.

}

/// Shared lowering code amongst all backends for doing ISLE-based lowering.
Expand All @@ -323,7 +323,7 @@ pub(crate) fn lower_common<C, F, I, const N: usize>(
) -> Result<(), ()>
where
C: LowerCtx,
[C::I; N]: smallvec::Array<Item = C::I>,
[(C::I, bool); N]: smallvec::Array<Item = (C::I, bool)>,
{
// TODO: reuse the ISLE context across lowerings so we can reuse its
// internal heap allocations.
Expand Down Expand Up @@ -367,7 +367,7 @@ where
renamer.add_rename(*temp, dst.to_reg(), *ty);
}
}
for inst in isle_ctx.emitted_insts.iter_mut() {
for (inst, _) in isle_ctx.emitted_insts.iter_mut() {
map_regs(inst, &renamer);
}

Expand All @@ -387,8 +387,12 @@ where
// Once everything is remapped we forward all emitted instructions to the
// `lower_ctx`. Note that this happens after the synthetic mov's above in
// case any of these instruction use those movs.
for inst in isle_ctx.emitted_insts {
lower_ctx.emit(inst);
for (inst, is_safepoint) in isle_ctx.emitted_insts {
if is_safepoint {
lower_ctx.emit_safepoint(inst);
} else {
lower_ctx.emit(inst);
}
}

Ok(())
Expand Down