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.

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

Implement loop peeling. #1510

Merged
merged 1 commit into from
Dec 19, 2024
Merged

Implement loop peeling. #1510

merged 1 commit into from
Dec 19, 2024

Conversation

ptersilie
Copy link
Contributor

@ptersilie ptersilie commented Dec 13, 2024

This implements a basic version of loop peeling. Note, that additional optimisations based on loop peeling, like common subexpression eliminiation, are not included in this PR. Those will be implemented in subsequent PRs.

@ptersilie ptersilie marked this pull request as draft December 13, 2024 14:18
@ltratt
Copy link
Contributor

ltratt commented Dec 16, 2024

On a fresh clone (and even if I merge in master), I see a different error:

thread '<unnamed>' panicked at ykrt/src/compile/jitc_yk/jit_ir/mod.rs:333:19:
index out of bounds: the len is 67 but the index is 69
stack backtrace:
   0: rust_begin_unwind
             at /rustc/5e1440ae514d98ddfcbf1607acb64d41e07ef616/library/std/src/panicking.rs:681:5
   1: core::panicking::panic_fmt
             at /rustc/5e1440ae514d98ddfcbf1607acb64d41e07ef616/library/core/src/panicking.rs:75:14
   2: core::panicking::panic_bounds_check
             at /rustc/5e1440ae514d98ddfcbf1607acb64d41e07ef616/library/core/src/panicking.rs:273:5
   3: <usize as core::slice::index::SliceIndex<[T]>>::index
             at /rustc/5e1440ae514d98ddfcbf1607acb64d41e07ef616/library/core/src/slice/index.rs:274:10
   4: core::slice::index::<impl core::ops::index::Index<I> for [T]>::index
             at /rustc/5e1440ae514d98ddfcbf1607acb64d41e07ef616/library/core/src/slice/index.rs:16:9
   5: <alloc::vec::Vec<T,A> as core::ops::index::Index<I>>::index
             at /rustc/5e1440ae514d98ddfcbf1607acb64d41e07ef616/library/alloc/src/vec/mod.rs:3346:9
   6: ykrt::compile::jitc_yk::jit_ir::Module::inst_raw
             at /vol/extra/ltratt/yk/ykrt/src/compile/jitc_yk/jit_ir/mod.rs:333:19
   7: ykrt::compile::jitc_yk::jit_ir::PackedOperand::unpack
             at /vol/extra/ltratt/yk/ykrt/src/compile/jitc_yk/jit_ir/mod.rs:1104:23
   8: ykrt::compile::jitc_yk::jit_ir::Inst::map_operand_locals
             at /vol/extra/ltratt/yk/ykrt/src/compile/jitc_yk/jit_ir/mod.rs:1588:21
   9: ykrt::compile::jitc_yk::jit_ir::dead_code::<impl ykrt::compile::jitc_yk::jit_ir::Module>::dead_code_elimination
             at /vol/extra/ltratt/yk/ykrt/src/compile/jitc_yk/jit_ir/dead_code.rs:20:17
  10: ykrt::compile::jitc_yk::opt::Opt::opt
             at /vol/extra/ltratt/yk/ykrt/src/compile/jitc_yk/opt/mod.rs:426:9
  11: ykrt::compile::jitc_yk::opt::opt
             at /vol/extra/ltratt/yk/ykrt/src/compile/jitc_yk/opt/mod.rs:567:5

@ltratt
Copy link
Contributor

ltratt commented Dec 18, 2024

We will need at least one (and preferably more) C tests for this: they don't necessarily have to be super-super complex, but we should at least check that jit-post-opt has peeled the loop!

@ltratt
Copy link
Contributor

ltratt commented Dec 18, 2024

Right, so just the missing C tests left I think.

@ptersilie
Copy link
Contributor Author

I've just added one simple test. I'm currently trying to make a C tests where we can see constant forwarding into the body, but I haven't had any luck so far.

@ltratt
Copy link
Contributor

ltratt commented Dec 18, 2024

I'm currently trying to make a C tests where we can see constant forwarding into the body, but I haven't had any luck so far.

I think yk_promote will do what you want.

@ptersilie
Copy link
Contributor Author

I think yk_promote will do what you want.

I tried all the promote tests. But it seems because we are using NOOPT_VAL they are always loaded from the stack and thus not passed into the body.

f: &F,
) -> Result<Self, CompilationError>
where
F: Fn(InstIdx, &Module) -> Operand,
Copy link
Contributor

Choose a reason for hiding this comment

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

Just noticed: this should be (&Module, InstIdx).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Inst::Tombstone => Inst::Tombstone,
Inst::TraceHeaderStart => {
// Copy the header label into the body while remapping the operands.
m.trace_body_start = m
Copy link
Contributor

Choose a reason for hiding this comment

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

This is almost certainly unsafe (similarly for TraceHeaderEnd below): it means that calling this method permanently mutates the global notion of TraceHeaderStart. If you call this method twice with a TraceHeaderStart, weird things are likely to happen.

IMHO: either users are forbidden from calling this function with TraceHeaderStart | TraceHeaderEnd or we work out some way of not mutating things globally.

@ltratt
Copy link
Contributor

ltratt commented Dec 19, 2024

Please squash.

@ptersilie
Copy link
Contributor Author

Squashed and rebased.

@ptersilie ptersilie marked this pull request as ready for review December 19, 2024 10:44
Co-authored-by: Laurence Tratt <laurie@tratt.net>
@ltratt ltratt added this pull request to the merge queue Dec 19, 2024
Merged via the queue into ykjit:master with commit 889dbdc Dec 19, 2024
2 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants