YJIT: Inline return address callback

[XrXr]

This makes it so that the generator and the output code read in the same
order. I think it reads better this way.

[jimmyhmiller]

I don't understand this code change. Maybe I'm missing something, but the code looks to be in the same order before inlining. The only difference here that I see is the change from BranchShape::Next0 | BranchShape::Next1 => unreachable!(), to _. I'm not sure I agree with that change. If we add a new branchshape it is often nice to know where we handle those things.

I'm just sincerely not sure what we gain from inlining the call here.

[XrXr]

Maybe I'm missing something, but the code looks to be in the same order before inlining.

If you read the generator in book order, you had to jump backwards in the file to line 4875 previously. After inlining gen_send_iseq() reads in execution order.

The _ part is a quirk with the API. It's more flexible than this usage needs, so I shifted it to hilight the intention that this is not really a branch and we only ever expect to get BranchShape::Default.

[jimmyhmiller]

I mean, that's true for all function calls. You have to go to a different location. Why inline this one?

I know some people think you should inline all simple use functions, personally not a fan. In this case we are inline into a 600 line mega function. If gen_return_branch isn't expected to be changing a bunch, I just don't see why inlining here is super useful.

If I were to inline anything here, it would be rather inline in gen_return_branch. Move the gen_branch call so that gen_return_branch looks like this:

fn gen_return_branch(
    jit: &mut JITState,
    asm: &mut Assembler,
    ocb: &mut OutlinedCb,
    return_block: BlockId,
    return_ctx: &Context,
) {
     // Write the JIT return address on the callee frame
     gen_branch(
        jit,
        asm,
        ocb,
        return_block,
        &return_ctx,
        None,
        None,
        |asm, target0, _target1, shape| {
            match shape {
                BranchShape::Default => {
                    asm.comment("update cfp->jit_return");
                    asm.mov(Opnd::mem(64, CFP, RUBY_OFFSET_CFP_JIT_RETURN), Opnd::const_ptr(target0.raw_ptr()));
                }
                _ => unreachable!()
            }
        },
    );
}

Now the call simplifies to

    gen_return_branch(jit, asm, ocb, return_block, &return_ctx);

Of course, with this change there is still a need to navigate in the code. But I guess I don't consider that operation to be one we do commonly. Whereas trying to scroll through and read gen_call_iseq to find the right place to put logic is something I've had to do often. I find that significantly easier when I don't need to read a ton of code to find the intention.

[maximecb]

@jimmyhmiller the reason to inline this is that the return branch is a bit of a special case. IMO it's an improvement.

[jimmyhmiller]

I'm not going to hold it up. If people want the change that's fine. But if it is a special case, to me that is more reason to isolate it to a function and explain why it is a special case. I still don't see how it is special, I'm sure it is, but this code change makes me see it as less of a special case.

[XrXr]

Extracting out to a different function makes sense when there are multiple callers, in this case there is a unique caller so it feels a bit heavy handed for my taste. Having a unique caller is really what's special about this site. Related to that, I think we would've inlined this if it was possible in C, but we were limited by the rules there. Regarding needing to read a bunch of code, that's why we leave comments in the code, that way you have something to anchor to when reading in book order and can choose to skip parts. The nice thing about things in book order is that the code is accessible/readable without help from Rust analyzer or other advanced editor capabilities -- you just scroll and the logic is right there.

[jimmyhmiller]

Extracting out to a different function makes sense when there are multiple callers, in this case there is a unique caller so it feels a bit heavy handed for my taste.

Yeah, I see that's where we differ. I find unique caller functions to often make code easier to understand and less prone to edit errors.

Only after I moved gen_branch inside of gen_return_branch did I notice that return_block is actually only used once and is derivable from the parameters already passed to gen_return_branch. So if I did that refactor, I'd actually pull in return_block.

The nice thing about things in book order is that the code is accessible/readable without help from Rust analyzer or other advanced editor capabilities -- you just scroll and the logic is right there.

Yeah, I get that. Though I personally just use rust analyzer. That said, the other answer that is pretty common in languages ecosystems that 1) prefer small functions and 2) don't have fancy tooling, is just to have a predictable place to put functions called by a single function. Usually write before or right after.

I'm only talking about this case because I think it's incredibly minor and I think useful for understanding your (@XrXr ) perspective on this. Are you opposed to unique caller functions in general? Is it case by case? Or do you have some guidelines you like to follow?

[XrXr]

Are you opposed to unique caller functions in general? Is it case by case? Or do you have some guidelines you like to follow?

It's case by case for me, with a strong preference for code being in execution order. I like to follow it for C code, too, but fewer tools to shape the code there (code end up like this, for example). By the way, I think what informs my ordering preference with codegen code in particular is that the API tries to look like assembly, and assembly files tend to have linear stretches (small building blocks).