Use a C-safe return type for __rust_[ui]128_* overflowing intrinsics

[tgross35]

Combined with [1], this will change the overflowing multiplication operations to return an extern "C"-safe type.

Link: rust-lang/compiler-builtins#735 [1]

[rustbot]

r? @wesleywiser

rustbot has assigned @wesleywiser.
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

[rustbot]

Some changes occurred in compiler/rustc_codegen_gcc

cc @antoyo, @GuillaumeGomez

Some changes occurred in compiler/rustc_codegen_cranelift

cc @bjorn3

[tgross35]

r? @bjorn3 @antoyo

There are a couple other places I need to update for GCC still.

[tgross35]

For cranelift, is there a way to load the stack slot so it can be combined with the integer for a CValue::by_val_pair @bjorn3?

[bjorn3]

You can use oflow_out_place.to_cvalue(fx).load_scalar(fx).

[rustbot]

These commits modify the library/Cargo.lock file. Unintentional changes to library/Cargo.lock can be introduced when switching branches and rebasing PRs.

If this was unintentional then you should revert the changes before this PR is merged.
Otherwise, you can ignore this comment.

[bors]

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

[bjorn3]

This doesn't apply to building the sysroot. Also the patch in library/Cargo.toml should be enough to apply while building a sysroot for cg_clif.

[bjorn3]

The lockfile changes should be reverted.

[tgross35]

Yeah, the entire second commit is just for testing and will be dropped. My plan is once you approve the Cranelift changes and somebody from GCC has looked that portion over, I will merge rust-lang/compiler-builtins#735, drop the second commit (which add my crates-iopatch), replace it with a normalcompiler-builtins` bump, and then this PR's merge will change everything atomically.

However, I am stuck completing the current bump at #135180, so I will hold off until that gets fixed.

[tgross35]

@rust-lang/wg-gcc-backend would one of you mind taking a look at the GCC changes? I am assuming that since this bump means the intrinsics are changing to get the same signature (type aside) as __mulosi4 they no longer need to be special cased. However, I am not very familiar with this code or whether it gets tested in CI somehow.

[antoyo]

From what I can see, this change doesn't look right as this will fall to calling self.overflow_call() below which expects that the overflow flag is returned from the function call (like these GCC intrinsics) whereas your change seems to return the integer result and not the overflow flag.

My guess is that you'll need to keep this special handling and change the method operation_with_overflow accordingly as these intrinsics are also used in src/intrinsic/mod.rs.

AFAIK, the only related tests that run in this CI are those lines.

[tgross35]

Thanks for taking a look. Doesn't this block currently sometimes use __mulosi4 without an early turn? After the change to compiler_builtins, __rust_*128_o should be consistent with the __mulo*i4 functions.

AFAIK, the only related tests that run in this CI are those lines.

Is there any kind of codegen/asm test that should be added here? Or is the existing test sufficient once CI gets that far.

[antoyo]

Thanks for taking a look. Doesn't this block currently sometimes use __mulosi4 without an early turn? After the change to compiler_builtins, __rust_*128_o should be consistent with the __mulo*i4 functions.

Indeed, it looks like there might be a bug in here.

Is there any kind of codegen/asm test that should be added here? Or is the existing test sufficient once CI gets that far.

What do you mean? There's more tests that are ran in the repo of rustc_codegen_gcc itself.

[tgross35]

Indeed, it looks like there might be a bug in here.

I changed this so mulo and the __rust functions should now be taking the same codepath, still have to update the rest.

Is there any kind of codegen/asm test that should be added here? Or is the existing test sufficient once CI gets that far.

What do you mean? There's more tests that are ran in the repo of rustc_codegen_gcc itself.

I am just wondering whether I should add another test anywhere to verify the new behavior, especially if __mulosi4 may have been called incorrectly before. And if so, what format/location this should be (for LLVM I'd probably add a codegen test, but I'm not sure what the equivalent would be here).

[tgross35]

@bjorn3 the is segfaulting at this line within __rust_i128_addo. The parameters look fine but it tries to mov %rcx,(%rdi) and store the oflow bool to the first argument.

rax            0x377cf7d5a29495da  3998343066125899226
rbx            0x0                 0
rcx            0x0                 0
rdx            0xa                 10
rsi            0x5b232dc60cb0dfd   410446438142250493
rdi            0x377cf7d5a29495da  3998343066125899226
rbp            0x7fffffffdb00      0x7fffffffdb00
rsp            0x7fffffffc9f0      0x7fffffffc9f0
r8             0x7fffffffda90      140737488345744
r9             0x5555555fefe0      93824992931808

[tgross35]

Actually that would be consistent with the return place before updating compiler-builtins, I wonder if the patched version isn't getting built for some reason.

Edit: yeah, I never remember that the version number needs to be higher in order for patches to work... tests pass locally now.

[bjorn3]

The cg_clif half LGTM. #135328 will have two minor conflicts with this PR though.