rustc_trans: rewrite mips64 ABI code #47964

jcowgill · 2018-02-02T10:30:37Z

This PR rewrites the ABI handling code for 64-bit MIPS and should fix various FFI issues including #47290.

To accomodate the 64-bit ABI I have had to add a new CastTarget variant which I've called Chunked (though maybe this isn't the best name). This allows an ABI to cast to some arbitrary structure of Reg types. This is required on MIPS which might need to cast to a structure containing a mixture of i64 and f64 types.

rust-highfive · 2018-02-02T10:30:43Z

Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @estebank (or someone else) soon.

If any changes to this PR are deemed necessary, please add them as extra commits. This ensures that the reviewer can see what has changed since they last reviewed the code. Due to the way GitHub handles out-of-date commits, this should also make it reasonably obvious what issues have or haven't been addressed. Large or tricky changes may require several passes of review and changes.

Please see the contribution instructions for more information.

Mark-Simulacrum · 2018-02-02T18:28:11Z

r? @eddyb

eddyb · 2018-02-02T21:33:53Z

Does clang need to do this? How many fields is that struct limited to? I'd rather not add the general case here.

eddyb · 2018-02-02T21:49:07Z

src/librustc_trans/cabi_mips64.rs

+                chunks.push(bits_to_int_reg(final_int_bits));
+            }
+
+            arg.cast_to(CastTarget::Chunked(chunks));


Ah, you don't need this! This is literally what Uniform { unit: Reg::i64(), total: trailing_int_size } does!

I think it's only equivalent to Uniform if chunks is empty when entering the else block.

Ah sorry I didn't see it came from more code above 😞.

jcowgill · 2018-02-03T13:23:26Z

Yes clang needs to do this. If you pass the structure directly to LLVM, it will allocate 1 register per field instead of merging them together into 64-bit chunks which is what happens here. There is no limit to the size of structures passed using this method. For example, if a function takes one structure argument containing 8 i64 and 8 f64 in any order, the entire structure will be passed in registers. Any extra fields get pushed onto the stack (after the early fields have been put in registers).

eddyb · 2018-02-03T21:49:38Z

src/librustc_trans/cabi_mips64.rs

+
+                    // We only care about aligned doubles
+                    if let layout::Abi::Scalar(ref scalar) = field.abi {
+                        if let layout::F64 = scalar.value {


How come this isn't recursive?

As in why doesn't it recurse into structures within structures?

I originally thought this from reading the ABI document, but it's a bit ambiguous and it seems the ABI is intentionally not recursive. Eg this structure is passed in an integer register by clang and gcc even though it is an aligned double.

struct test { struct { double x; }; };

eddyb · 2018-02-03T21:51:16Z

@jcowgill So only a prefix actually gets put into registers? How many registers are there to use? (is this something we can rely on, like we do on x64?)

jcowgill · 2018-02-04T10:33:15Z

Yes only a prefix is put into registers. My earlier comment about "8 i64 and 8 f64" was wrong - only the first 8 "chunks" are put into registers (for a total of 8 registers of either type being used). Everything after the 64th byte will be put on the stack.

eddyb · 2018-02-04T10:51:06Z

@jcowgill Thanks for investigating this! I'm still not super eager to merge the version using a Vec, but maybe there's a formulation that can also cheaply supersede the CastTarget::Pair x64 uses.

How do you feel about ChunkedPrefix { prefix: [RegKind; 8], chunk: Size, total: Size }?

I might also want to maybe move towards a model that better match hardware register sizes, so we wouldn't need to store the 64-bit size of the used registers (because they can't be anything else).

jcowgill · 2018-02-05T15:28:26Z

How do you feel about ChunkedPrefix { prefix: [RegKind; 8], chunk: Size, total: Size }?

I think that would work, although it is a bit more complicated. Would this be implemented using a separate structure the same way Uniform is?

If I understand correctly, these would then also be equivalent?
Uniform { unit: unit, total: total }
ChunkedPrefix { prefix: [unit.kind; 8], chunk: unit.size, total: total }

eddyb · 2018-02-05T15:39:59Z

I meant only a CastTarget variant (with named fields), not separate structure. To also combine the CastTarget::Uniform variant into this one you'd want to replace total: Sized with rest: Uniform.

jcowgill · 2018-02-05T16:51:35Z

I think my equivalence is wrong anyway since it doesn't work for floats. I don't like rest: Uniform because it would have to store the total size for the entire cast target instead of the size of the "rest" part.

If ChunkedPrefix is supposed to only be a variant with named fields, I'm not sure why Uniform gets its own separate structure.

I'll have a go at implementing this...

eddyb · 2018-02-05T17:32:14Z

@jcowgill I thought Uniform was separate because it was used elsewhere but if that's not true at all we can probably inline its methods into CastTarget's?

jcowgill · 2018-02-08T13:06:30Z

I've pushed a new version using ChunkedPrefix.

eddyb · 2018-02-08T14:41:48Z

src/librustc_trans/abi.rs

+                cx.data_layout().aggregate_align
+                    .max(Reg { kind: RegKind::Integer, size: chunk }.align(cx))
+                    .max(Reg { kind: RegKind::Float, size: chunk }.align(cx))
+                    .max(Reg { kind: RegKind::Vector, size: chunk }.align(cx))


I'd rather iterate the array and set 3 bool flags - some of these might not be used.

eddyb · 2018-02-08T14:46:16Z

src/librustc_trans/cabi_mips64.rs

    let dl = &cx.tcx.data_layout;
    let size = arg.layout.size;
-    let align = arg.layout.align.max(dl.i32_align).min(dl.i64_align);

    if arg.layout.is_aggregate() {


Should handle the simple (non-aggregate) case first with an early return, to avoid the indentation in the aggregate case.

eddyb · 2018-02-08T14:48:26Z

src/librustc_trans/cabi_mips64.rs

-use rustc::ty::layout::Size;
+fn extend_integer_width_mips(arg: &mut ArgType, bits: u64) {
+    // This is like ArgType::extend_integer_with_to, but always sign extends
+    // integers >= 32 bits as required by the n64 ABI


This is pretty confusing - so specifically, u32 is the only value that gets passed differently? Can you use extend_integer_with_to except in the u32 case? That way it's more obvious where the exception lies.

Ok I've adjusted the code to do that.

eddyb · 2018-02-08T14:51:04Z

src/librustc_trans/cabi_mips64.rs

+                        },
+                        _ => None
+                    }
+                }).collect();


Can you do this without a Vec? e.g. you can have a variable with the mutable iterator and call .next() three times, checking for combinations of Some and None.

I've refactored the float reg testing code into a separate function which I call manually for the different numbers of fields. I think it looks a bit better.

eddyb · 2018-02-08T14:52:31Z

src/librustc_trans/cabi_mips64.rs

+
+            if ret.layout.fields.count() == float_regs.len() {
+                if float_regs.len() == 1 {
+                    ret.cast_to(Uniform { unit: float_regs[0], total: float_regs[0].size });


We have conversion impls so you can specifically do ret.cast_to(float_regs[0]).

eddyb · 2018-02-08T14:53:25Z

src/librustc_trans/cabi_mips64.rs

+                                // Insert enough integers to cover [last_offset, offset)
+                                assert!(last_offset.is_abi_aligned(dl.f64_align));
+                                for _ in 0..((offset - last_offset).bits() / 64) {
+                                    chunks.push(RegKind::Integer);


You can keep an index into the final array to avoid allocating a Vec.

eddyb · 2018-02-08T14:54:28Z

src/librustc_trans/cabi_mips64.rs

            total: size
        });
-        if !offset.is_abi_aligned(align) {


There's no alignment padding anymore? I'm not even sure what clang does wrt that for other targets - especially since there's a stackalign attribute now.

I think this is correct. In 32-bit MIPS, we have to pad 64-bit aligned structures manually because we cast to an i32 Uniform so LLVM doesn't know that the structure is misaligned. In 64-bit MIPS we always use i64 chunks so this issue doesn't arise.

eddyb · 2018-02-08T15:00:12Z

src/librustc_trans/abi.rs

@@ -407,7 +407,8 @@ impl<'tcx> LayoutExt<'tcx> for TyLayout<'tcx> {
 #[derive(Clone, Copy, PartialEq, Eq, Debug)]
 pub enum CastTarget {
    Uniform(Uniform),
-    Pair(Reg, Reg)
+    Pair(Reg, Reg),
+    ChunkedPrefix { prefix: [RegKind; 8], chunk: Size, total: Size }


Really starting to wonder if we can turn all 3 variants into one (such that CastTarget becomes a struct).

If the prefix array uses Option<RegKind> instead to indicate that not all 8 slots are full, then we can replace total with rest: Uniform and have it represent strictly what's after the prefix.

And then Pair(a, b) can become prefix: [Some(a.kind), None, ..., None], chunk: a.size, rest: Uniform::from(b).

Oh and chunk makes more sense as prefix_chunk.

I've attempted to implement this (in separate commits for the moment). It seems to work ok.

pietroalbini · 2018-02-19T13:59:45Z

@eddyb the user pushed some commits that seems to have fixed your concerns, can you review this again?

eddyb · 2018-02-22T00:14:54Z

src/librustc_trans/abi.rs

+            // Simplify to a single unit when there is no prefix and size <= unit size
+            if self.rest.total <= self.rest.unit.size {
+                return rest_ll_unit;
+            }


Does Uniform need to be a separate struct at this point? Probably not, but we can revisit later.

eddyb · 2018-02-22T00:17:49Z

Apologies for the delay, still digging through my notifications. This new CastTarget looks great!

@bors r+

bors · 2018-02-22T00:17:51Z

📌 Commit 47c33f7 has been approved by eddyb

@nrc

Fixes rust-lang#47311. r? @nrc

rustc_trans: rewrite mips64 ABI code This PR rewrites the ABI handling code for 64-bit MIPS and should fix various FFI issues including rust-lang#47290. To accomodate the 64-bit ABI I have had to add a new `CastTarget` variant which I've called `Chunked` (though maybe this isn't the best name). This allows an ABI to cast to some arbitrary structure of `Reg` types. This is required on MIPS which might need to cast to a structure containing a mixture of `i64` and `f64` types.

Rollup of 17 pull requests - Successful merges: #47964, #47970, #48076, #48115, #48166, #48281, #48297, #48302, #48362, #48369, #48489, #48491, #48494, #48517, #48529, #48235, #48330 - Failed merges:

gnzlbg · 2018-02-28T10:37:54Z

@jcowgill I see you have a bunch of MIPS-related PRs. We are currently having trouble implementing MIPS SIMD intrinsics in coresimd (we can't compile binaries with the msa feature enabled). Let me know if you would be interested to help with that.

jcowgill · 2018-02-28T16:44:59Z

@gnzlbg I can probably help with that - assuming I find some time :)
I see this issue (maybe we could move to it): rust-lang/stdarch#170. Is there any existing mips work?

gnzlbg · 2018-02-28T20:39:50Z

@jcowgill this would be a start: #46310

rust-lang/stdarch#170. Is there any existing mips work?

I started implementing those intrinsics, but because of the issue above, did not progress much. Maybe once that issue is solved, implementing the intrinsics will "just work".

Since rust-lang#47964 was merged, 64-bit mips started passing all structures using 64-bit chunks regardless of their contents. The repr-transparent-aggregates tests needs updating to cope with this.

rust-highfive assigned estebank Feb 2, 2018

eddyb reviewed Feb 2, 2018

View reviewed changes

shepmaster added the S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. label Feb 3, 2018

eddyb reviewed Feb 3, 2018

View reviewed changes

estebank assigned eddyb and unassigned estebank Feb 5, 2018

jcowgill force-pushed the mips64-abi branch from 035af6b to a8df383 Compare February 8, 2018 11:26

eddyb reviewed Feb 8, 2018

View reviewed changes

jcowgill added 3 commits February 13, 2018 13:40

rustc_trans: add abi::CastTarget::ChunkedPrefix

f45a474

rustc_trans: rewrite mips64 abi

68042ba

rustc_trans: add chunked prefix fields to CastTarget

05d66dc

rustc_trans: adjust mips64 abi to use new CastTarget

47c33f7

jcowgill force-pushed the mips64-abi branch from a8df383 to 47c33f7 Compare February 14, 2018 15:05

pietroalbini added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Feb 14, 2018

eddyb reviewed Feb 22, 2018

View reviewed changes

bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Feb 22, 2018

Manishearth added a commit to Manishearth/rust that referenced this pull request Feb 25, 2018

Rollup merge of rust-lang#47964 - jcowgill:mips64-abi, r=eddyb

66aa98c

Fixes rust-lang#47311. r? @nrc

kennytm mentioned this pull request Feb 25, 2018

Rollup of 17 pull requests #48531

Merged

bors merged commit 47c33f7 into rust-lang:master Feb 25, 2018

jcowgill mentioned this pull request Mar 6, 2018

FFI aggregate handling for mips64 is broken #47290

Closed

jcowgill deleted the mips64-abi branch March 8, 2018 17:33

rustc_trans: rewrite mips64 ABI code #47964

rustc_trans: rewrite mips64 ABI code #47964

Conversation

jcowgill commented Feb 2, 2018

rust-highfive commented Feb 2, 2018

Mark-Simulacrum commented Feb 2, 2018

eddyb commented Feb 2, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jcowgill commented Feb 3, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

eddyb commented Feb 3, 2018

jcowgill commented Feb 4, 2018

eddyb commented Feb 4, 2018 • edited Loading

jcowgill commented Feb 5, 2018

eddyb commented Feb 5, 2018

jcowgill commented Feb 5, 2018

eddyb commented Feb 5, 2018

jcowgill commented Feb 8, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pietroalbini commented Feb 19, 2018

Choose a reason for hiding this comment

eddyb commented Feb 22, 2018

bors commented Feb 22, 2018

gnzlbg commented Feb 28, 2018

jcowgill commented Feb 28, 2018

gnzlbg commented Feb 28, 2018

eddyb commented Feb 4, 2018 •

edited

Loading