Assert that all promotion data has been consumed.

[ltratt]

One thing that I realised is that if we didn't consume all promotion data -- e.g. because we forgot to consume it when we're outlining -- weird things would happen. This assert always succeeds on my tests, but I think it's worth having in.

[vext01]

Good idea!

[ltratt]

This is interesting:

16:49:40 .thread '<unnamed>' panicked at ykrt/src/compile/jitc_yk/trace_builder.rs:1299:9:
16:49:40 assertion `left == right` failed
16:49:40 left: 48
16:49:40 right: 60
16:49:40 note: run with `RUST_BACKTRACE=1` environment variable to display a backtrace
16:49:40 thread '<unnamed>' panicked at ykrt/src/mt.rs:337:46:

That suggests we already have a test case where we aren't consuming all promotions as we would expect to.

@ptersilie I think this is one for you when you're back.

[ptersilie]

I might have an idea what's going on here, though I haven't confirmed this yet. From what I understand, we collect a list of all constants promoted during tracing and then this check makes sure we use all promotions when constructing the trace. What I believe this is forgetting is that we don't process everything we see during tracing. For example if there's a function containing a loop that also contains promotions, we will later outline that function and thus not process those promotions during trace building.

I will try constructing a test using this information to confirm.

[ltratt]

For example if there's a function containing a loop that also contains promotions, we will later outline that function and thus not process those promotions during trace building.

Right, this is my guess too: but notice that we should still be advancing promotion_idx in this case!

So I think the assert should always be true, because what it says is "we got to the end of the promotion data" (which, I now realise, isn't quite the same as "we consumed all the promotion data" implies).

[ptersilie]

This means that even when outlining functions/blocks, we will need to iterate through them to find promotions and advance promote_idx. It's going to be a bit annoying as currently we simply skip outlining blocks without looking at them at all.

[ltratt]

I must admit that I'm slightly surprised we do get a yk_promote in an outline function: it might be worth checking how this happens. A first thought is that this might be because in lua we get a function call which calls an outlined function (which one?) that then recalls the main interpreter loop. If that is the case, it means we're not inlining Lua-level functions, which would be less than ideal in yklua.

So one way of thinking of this in the short term is to say "if we haven't got to the end of the promotion data, the user should change their interpreter to make sure yk_promote isn't (directly or indirectly) contained in an outlinable function".

[ltratt]

This LGTM but I think @vext01 needs to take over reviewing now.

[ltratt]

And I suggest before merge we squash the two commits into one, so we don't leave a "this breaks" commit in history. Lukas can do a co-authored-by!

[vext01]

Oopsy. An i1 would report a byte size of 0! I know we can't promote an i1, but still.

See this jit ir routine for inspiration.

Every time we are tempted to divide a bit size by 8, an alarm bell should ring :)

[ptersilie]

I just copied this from handle_operand. We should fix it there too, but I'm not sure this PR is the right place.

[vext01]

OK, please follow this PR up with the fix. It'll surely bite us later (again).

[vext01]

Seems good, modulo that one comment.

[ltratt]

Fixes #1440.

[ptersilie]

Fixes #1440.

I think you meant to comment that here: #1461

[ltratt]

Uh, yeah.

[ptersilie]

I need to squash first.

[vext01]

please squash.

[ptersilie]

Squashed.

[ptersilie]

I realised that the handle_operand bit doesn't have the same issue since it matches the bits. So decided to make this change in this PR after all. Ok to squash?

[vext01]

go ahead.

[ptersilie]

Squashed.

[vext01]

You can force push the clippy fix.

[ptersilie]

Ready.

[ptersilie]

Unexpected HotLocationKind

Huh. I thought I fixed that.

[ltratt]

@ptersilie I thought we'd got rid of this code:

        let root_ctr = match &hl.kind {
            HotLocationKind::Compiled(root_ctr) => Arc::clone(root_ctr)
                .as_any()
                .downcast::<X64CompiledTrace>()
                .unwrap(),
            HotLocationKind::SideTracing { root_ctr, .. } => Arc::clone(root_ctr)
                .as_any()
                .downcast::<X64CompiledTrace>()
                .unwrap(),
            _ => panic!("Unexpected HotLocationKind"),
        };

but obviously we didn't! Do we even need it? There's no way of making this code correct AFAICS: it's broken.

[ltratt]

After offline discussion we realised this is a bit of code duplication.

[ltratt]

@vext01 If/when #1464 merges, this PR can hopefully be merged.