[beetrees]

View all comments

When LLVM intrinsics return multiple values, they return them in an LLVM struct. As Rust inserts explicit padding when creating the LLVM type of a Rust struct, stdarch currently uses packed structs to generate a similar (packed) struct and then re-packs the struct in autocast. This PR instead creates the struct from the fields in the struct/tuple directly - this allows the use of regular Rust structs and tuples to work as expected, and also removes the dependency on the internals of how Rust converts structs to LLVM types.

As link_llvm_intrinsics is an internal feature I was unsure what the correct approach to error handling was - currently I've just used bug! if the return type is invalid, but I can change it to use proper errors if that's preferred.

[rustbot]

r? @TaKO8Ki

rustbot has assigned @TaKO8Ki.
They will have a look at your PR within the next two weeks and either review your PR or reassign to another reviewer.

Use r? to explicitly pick a reviewer

Why was this reviewer chosen?

The reviewer was selected based on:

• Owners of files modified in this PR: compiler
• compiler expanded to 77 candidates
• Random selection from 17 candidates

[bjorn3]

Why was the matching of SSA value names removed? That makes it less robust against future changes, right?

View changes since the review

[beetrees]

Not really IMO. The numbered LLVM variables are always in sequential order, so any change to the names would involve reordering the statements or adding new statements inbetween, which would cause the test to fail regardless.

[bjorn3]

Adding new statements didn't require rewriting everything after it previously.

[beetrees]

I see what you mean (additionally the original used CHECK whereas I've used CHECK-NEXT). I don't think that extra flexibility is wanted - if e.g. an extra store statement gets added between the existing statements then the test should fail. If the change is desired, it's easy enough to update the test with the new IR.

[bjorn3]

As link_llvm_intrinsics is an internal feature I was unsure what the correct approach to error handling was - currently I've just used bug! if the return type is invalid, but I can change it to use proper errors if that's preferred.

bug!() seems fine to me.

[beetrees]

Figured out a better way to do this, I'll mark as a draft for now.

[beetrees]

Ready for review.

[rustbot]

rustc_codegen_gcc is developed in its own repository. If possible, consider making this change to rust-lang/rustc_codegen_gcc instead.

cc @antoyo, @GuillaumeGomez

[antoyo]

Please do not use ReturnSlot here as it is specifically meant to support the sret ABI as mentioned in its doc-comment.
I do not want to add a second meaning to ReturnSlot to avoid silently change the ABI when not intended.

View changes since the review

[beetrees]

I've changed it to an Option called memory_return_pointer.

[antoyo]

There's still a risk of messing up the ABI since we have a ReturnSlot here.
I guess the risk is low, but would there be an easy way to avoid using ReturnSlot here?

View changes since the review

[beetrees]

Not without changing the make_return_dest function to not use it, or duplicating the code. The PassMode used to decide whether a return slot is needed is constructed directly above the call, and I've added an assert straight afterwards to check that return_slot has the expected variant. I think that should mitigate the risks?

[antoyo]

My worry is more about using that return_slot in a call to bx.call(), which would change the ABI: that's what I mean by it is error-prone to use ReturnSlot here.
This assert does not prevent that.
Given that, do you see another solution to avoid using ReturnSlot here?
Would you really need to duplicate the whole code of make_return_dest? Duplicating a few lines of code here seems less of an issue than having the possibility of silently changing the ABI when it is not intended.

[beetrees]

If the issue is the risk of someone passing return_slot to bx.call() in the future, would putting the whole thing in it's own block with a comment help mitigate your concern (apologies if I'm misunderstanding something)? e.g.:

let (return_dest, memory_return_pointer) = {
    // This `return_slot` is just for getting a pointer to an appropriate place to store a `BackendRepr::Memory`
    // result, and must not be passed used for anything else (such as `bx.call`).
    let (return_dest, return_slot) = self.make_return_dest(bx, destination, &return_abi);
    (return_dest, match return_slot {
        ReturnSlot::Direct => None,
        ReturnSlot::Indirect(pointer) => Some(pointer),
    })
}

I really don't want to duplicate make_return_dest as that means that any future changes to that function (e.g. changes to the way MIR locals are stored) will have to be kept in sync with the copy (or fall out of sync, causing bugs). Alternatively I could refactor make_return_dest into make_return_dest_inner which would be exactly the same but return (ReturnDest, Option<Value>) instead of (ReturnDest, ReturnSlot) - make_return_dest would then just be a thin wrapper around make_return_dest_inner.

[antoyo]

I'd still prefer to keep the type-level safety we have here (the ReturnSlot type that handles the sret ABI) instead of a comment.
As I understand, you don't use the sret ABI for LLVM intrinsics, right?
If that is the case, I would have assume you don't need to copy the whole content of make_return_dest, but I'm not familiar with this code, so I might be wrong.
If you cannot find a way to do so, I guess a thin wrapper over make_return_dest would be a good option, with a comment to explain that this is to hide the return_slot from the caller.

[beetrees]

I've separate out the logic into make_return_place (which uses Option<PlaceValue> instead of ReturnSlot).

[rustbot]

This PR was rebased onto a different main commit. Here's a range-diff highlighting what actually changed.

Rebasing is a normal part of keeping PRs up to date, so no action is needed—this note is just to help reviewers.