ykjit / yk

yk packages
https://ykjit.github.io/yk/
Other
31 stars 7 forks source link

The way unit tests assign locations to load_ti insuctions needs overhaul #1425

Open vext01 opened 3 days ago

vext01 commented 3 days ago

I started writing a test like this:

    #[test]
    fn cg_and() {
        codegen_and_test(
            "
              entry:
                %0: i16 = load_ti 0
                %1: i16 = load_ti 1
                %2: i32 = load_ti 2
                %3: i32 = load_ti 3
                %4: i56 = load_ti 4
                %5: i56 = load_ti 5
                %6: i16 = and %0, %1
                %7: i32 = and %2, %3
                %8: i56 = and %4, %5
            ",
            "
                ...
                ; %6: i16 = and %0, %1
                xxx
                ; %7: i32 = and %2, %3
                xxx
                ; %8: i56 = and %4, %5
                xxx
                ...
                ",
        );
    }

And got a crash like this:

thread 'compile::jitc_yk::codegen::x64::tests::cg_and' panicked at ykrt/src/compile/jitc_yk/codegen/x64/mod.rs:932:17:
not yet implemented: Register(6, 4, 0, [])

The parser has eventually assigned a variable to register 6 (RBP, the base pointer). If you naively implement the missing case in cg_loadti then the register allocator understandably gets upset, since RBP is reserved.

@ptersilie and I took a brief look at how the JIT IR parser is allocating varlocations, it naively starts allocating trace inputs starting at register number 3 and incrementing the number each time. Further, I don't think it takes register class into account, nor starts spilling if you run out of registers.

Of course, if your test has enough variables, then eventually you allocate RBP and boom.

Discussion with @ptersilie suggests that to do this properly, we might want the parser to accept syntax like:

%1: i32 = load_ti rax
%2: float = load_ti [rbp - 8]

Assigning @ptersilie for now, but I appreciate he won't get to this soon. Unless @ltratt is looking for a distraction?

ltratt commented 3 days ago

Discussion with @ptersilie suggests that to do this properly, we might want the parser to accept syntax like:

There are (at least) the following problems here where our past is catching up with us:

  1. we have a gigantic ever-sprawling mess where details of yksmp (which encodes platform specific details) leak outwards: we convert back and forth in various places where we should probably not. The first thing to do is to clean up yksmp (e.g. move from anonymous tuples to named struct fields), and then see what we can do to abstract away from yksmp inside the rest of yk.
  2. most JIT IR tests should not reference machine registers.
  3. the JIT IR parser has to hack around (1) by handing out these "guessed" registers.
  4. I still hope we will move to something more like RPython's syntax where instead of the "load_ti" noise, we have a list of parameters in one line at the start of a trace / jump location.

That said, there's a -- horrible hack, but we have so many now, what's one more? -- way of fixing the IR parser: just make it skip register 6. It will still eventually run out of registers, but c'est la vie.

vext01 commented 3 days ago

just make it skip register 6

Yes, that's one thing I proposed to @ptersilie -- we could also check if we run out of registers.

This still wouldn't let us allocate a FP reg, but it will get me going.

ltratt commented 3 days ago

load_ti can check the assignment type and if it's an FP register, allocate an FP register.

vext01 commented 3 days ago

Yeah, it could.

OK, so I will put my other work on hold to hack this into shape.

I certainly agree that we need to rethink the whole thing. If we can get rid of load_ti, then fantastic!

vext01 commented 17 hours ago

Worked around this in https://github.com/ykjit/yk/commit/40fc3ddb9d48415440c6bf436f23dec0cdee2ce7