NVPTX: Avoid PassMode::Direct for args in C abi

[kjetilkjeka]

Fixes #117480

I must admit that I'm confused about PassMode altogether, is there a good sum-up threads for this anywhere? I'm especially confused about how "indirect" and "byval" goes together. To me it seems like "indirect" basically means "use a indirection through a pointer", while "byval" basically means "do not use indirection through a pointer".

The return used to keep PassMode::Direct for small aggregates. It turns out that make_indirect messes up the tests and one way to fix it is to keep PassMode::Direct for all aggregates. I have mostly seen this PassMode mentioned for args. Is it also a problem for returns? When experimenting with byval as an alternative i ran into this assert

I have added tests for the same kind of types that is already tested for the "ptx-kernel" abi. The tests cannot be enabled until something like #117458 is completed and merged.

CC: @RalfJung since you seem to be the expert on this and have already helped me out tremendously

CC: @RDambrosio016 in case this influence your work on rustc_codegen_nvvm

@rustbot label +O-NVPTX

[rustbot]

r? @compiler-errors

(rustbot has picked a reviewer for you, use r? to override)

[kjetilkjeka]

@rustbot label +O-NVPTX

[RalfJung]

I must admit that I'm confused about PassMode altogether, is there a good sum-up threads for this anywhere?

Sadly, no. The best we have are the API docs. I improved them a bit recently, but we are still lacking some proper "big-picture" docs for this. largely this is because while the system was designed with some big-picture ideas ~5 years ago, those were not written down at the time and since then it's been hotfixed and patched without regard for a coherent story. The entire system needs an overhaul. See rust-lang/compiler-team#672: the compiler team accepted the proposal to overhaul this, but we're still looking for a volunteer to do the work.

CC: @RalfJung since you seem to be the expert on this and have already helped me out tremendously

😂 I knew nothing about this until around 3 months ago, and spent way too much time on it since.^^ I still don't feel like an expert, since all I know is reverse engineered from looking at parts of rustc. There are many things about ABI I do not know. But at this point I think I know at least how rustc deals with ABI, and I know some necessary (but not sufficient) conditions for ABI adjustments to make sense.

[RalfJung]

Sadly, for this PR, I can't really tell if it makes any sense. But if it helps fix the assertions and also still generates working code for your tests, then it's at least an improvement...

[bjorn3]

I think you will want to use PassMode::Cast for aggregate types.

[kjetilkjeka]

I did experiment with casting to Uniform with Reg size equal to alignment and size equal to size. This caused miscompilation when passing types like

#[repr(C)]
pub struct SingleU8 {
    f: u8,
}

The problem was that since it was passed as a single register of size 8bit it passed it like a primitive type u8 instead of an aggregate. In ptx these primitives are padded while aggregates are not.

[bjorn3]

In ptx these primitives are padded while aggregates are not.

😱

[compiler-errors]

Sorry, too busy to review this 😰

r? compiler

[cjgillot]

r? compiler

[bors]

☔ The latest upstream changes (presumably #117500) made this pull request unmergeable. Please resolve the merge conflicts.

[davidtwco]

Apologies for the delay in getting to this - we don't have an expert in NVPTX on the team, so I'll take a stab at reviewing this so it isn't stuck forever. It seems reasonable to me.

Does this mean that NVPTX can be re-enabled in tests/ui/abi/compatibility.rs now? If so, could you do that in this PR? Otherwise let me know and I'll approve this.

[kjetilkjeka]

When rebasing there are unfortunately regressions on some of the tests related to returning structs. I will look into re-enabling the compatibility test when I have investigated these regressions.

If there's any silver lining I guess it is that it highlights the importance of #117458 and getting the nvptx tests up and running again in CI.

[RalfJung]

Does this mean that NVPTX can be re-enabled in tests/ui/abi/compatibility.rs now? If so, could you do that in this PR? Otherwise let me know and I'll approve this.

Yeah it should be re-enabled, otherwise the claim that it fixes the issue is not accurate.

[apiraino]

Checking status of this PR. @kjetilkjeka is this PR waiting on #117458? Switching the review flag for a rebase, thanks.

@rustbot author

[kjetilkjeka]

Yes, this is waiting for #117458

It turned out to be more difficult than anticipated to get correct. I will of course spend the time to figure it out, but when I manage to get it right I would hate for it to break again. Therefore I would love to wait until the nvptx tests are enabled in CI so I can write tests and be confident that things will keep working.

[OwenTrokeBillard]

Is this still blocked now that #117458 is merged?

[apiraino]

@kjetilkjeka when you have a sec. can you look at this patch again? Thanks

@rustbot author

[kjetilkjeka]

It's unblocked now 🎆 . I will take a look at this at the end of this week or early next week.

[kjetilkjeka]

After looking into this my conclusions is that it's impossible to both avoid PassMode::Direct for the C ABI and generate correct ptx.

An example that illustrates this is returning a type in in this way

#[repr(C)]
pub struct SingleU8 {
    f: u8,
}

#[no_mangle]
pub unsafe extern "C" fn f_single_u8_ret() -> SingleU8 {
    SingleU8 { f: 42 }
}

Which should give the following signature in ptx.

.visible .func  (.param .align 1 .b8 func_retval0[1]) f_single_u8_ret()

The only two PassModes that give the correct signature is Direct and Indirect{on_stack: true}. For arguments it's possible to use Indirect{on_stack: true} but that is not the case for returns.

PassMode::Cast casted to an uniform does the correct thing for many types, but unfortunately not for single element structs (it passes like it was the underlying type, not the struct itself).

The solution is probably to have something like the CAbiType described in this issue and a PassMode::CAbiType(CAbiType) that can pass like a concrete C type. But everything related to this ABI compatibility things is unfortunately too complex for me to handle.

That means that this PR avoids the use of PassMode::Direct in argument position. But unfortunately doesn't manage to solve it in return position. I have added tests for both return and argument position, but the test for return position is ignored.

[davidtwco]

This seems like an improvement even if it doesn't fix everything so LGTM.

[davidtwco]

@bors r+ rollup

[bors]

📌 Commit ead02ba has been approved by davidtwco

It is now in the queue for this repository.

[RalfJung]

Unfortunately this did not fully fix the problem, see #117480 (comment).

[RalfJung]

Why does this have to be ignored? You are still using PassMode::Direct for the return value, so I thought that would still generate the right ABI?

[kjetilkjeka]

It's unfortunately slightly broken (from before this PR) since there never were any tests for this.

I will enable these tests in the follow up PR we talked about that I hopefully will be able to create today

[kjetilkjeka]

Enabled in #125980