RISC-V Codegen Problem with type information

[coastalwhite]

Codegen for riscv64gc-unknown-linux-gnu sends wrong type information to LLVM.

When I compile on godbolt.org with the following flags.

GodBolt Link

--target=riscv64gc-unknown-linux-gnu -C target-feature=+zbb -C opt-level=3

I tried this code:

fn max(a: u32, b: u32) -> u32 {
    u32::max(a, b)
}

It outputs:

example::max:
        sext.w  a1, a1
        sext.w  a0, a0
        maxu    a0, a0, a1
        ret

It definitely does not need to sign extend a0 or a1 and in fact, when we inspect the LLVM IR, it seems to assume that the numbers are i32s instead of u32s. Is this a problem with Rust's codegen output?

LLVM IR:

; example::max
define noundef i32 @example::max(i32 noundef %a, i32 noundef %b) unnamed_addr personality ptr @rust_eh_personality {
start:
  %.0.sroa.speculated.i = tail call i32 @llvm.umax.i32(i32 %a, i32 %b)
  ret i32 %.0.sroa.speculated.i
}

declare noundef signext i32 @rust_eh_personality(i32 noundef signext, i32 noundef signext, i64 noundef, ptr noundef, ptr noundef) unnamed_addr #1

declare i32 @llvm.umax.i32(i32, i32) #2

Properly optimized code would be:

example::max:
        maxu    a0, a0, a1
        ret

[Urgau]

when we inspect the LLVM IR, it seems to assume that the numbers are i32s instead of u32s. Is this a problem with Rust's codegen output?

LLVM integers types do not differentiate between signed or unsigned integers. It's the operation (ie the intrinsic) that makes the differentiation and llvm.umax.* is the unsigned max variant:

Return the smaller of %a and %b comparing the values as unsigned integers

It's seems to me that it's a problem with the RISC-V LLVM backend.

@rustbot labels: +A-LLVM +A-codegen +O-riscv +T-compiler -needs-triage

[rakicaleksandar1999]

According to the documentation, for RV64, the registers are 64 bits wide. The arguments of the @llvm.umax.i32 intrinsic are 32 bits wide, and the higher 32 bits of the registers a0 and a1 are practically unknown. The maxu instruction probably uses all of the 64 bits of the registers, so it is necessary to ensure that the higher 32 bits of the 64 bits wide registers have appropriate values, I would say.

[nikic]

The problem here is that we don't set signext/zeroext attributes on the arguments with the Rust ABI. signext is set when using extern "C" (as it is an ABI requirement there). It probably makes sense to set them for the Rust ABI as well, for this target.

[workingjubilee]

cc @kito-cheng @michaelmaitland @robin-randhawa-sifive Please triage and/or address this issue.

[michaelmaitland]

I don't think there is a problem here.

I think this statement is incorrect.

Properly optimized code would be:

example::max:
      maxu    a0, a0, a1
      ret

As @rakicaleksandar1999 points out, on RV64, the registers are 64 bits wide. The maxu instruction considers the entire 64bit register size in its calculation. Therefore, we need the sign extends. On RISC-V, some instructions have w variants which allow us to consider only the lower 32 bits. There is no w variant of the maxu instruction.

I think we should be able to close this issue.

[workingjubilee]

@michaelmaitland

Hm, this does compile to just maxu with the extern "C" ABI, though. Godbolt

pub extern "C" fn max(a: u32 , b: u32) -> u32 {
    u32::max(a, b)
}

becomes

example::max::hd12728cb718981ea:
        maxu    a0, a0, a1
        ret

[SpriteOvO]

C++ can also be compiled into just one maxu.

https://godbolt.org/z/x9dv5v1h3

uint32_t max(uint32_t x, uint32_t y) {
    return std::max(x, y);
}

max(unsigned int, unsigned int):
        maxu    a0,a0,a1
        ret

[michaelmaitland]

Can rust generate signext or zeroext on the function arguments? https://godbolt.org/z/x6vsvP156

I think this should fix the problem.

[topperc]

The problem here is that we don't set signext/zeroext attributes on the arguments with the Rust ABI. signext is set when using extern "C" (as it is an ABI requirement there). It probably makes sense to set them for the Rust ABI as well, for this target.

@nikic where is the distinction between Rust ABI and extern "C" made?

[workingjubilee]

All of the relevant code should be in

• rustc_target/src/abi/mod.rs
• rustc_target/src/abi/call/riscv.rs

But if it's not it may be somewhere in:

• rustc_abi
• rustc_codegen_llvm
• rustc_codegen_ssa

And rustc will indeed generate signext or whatnot if we attach the relevant ArgExtension:

rustc_codegen_llvm/src/abi.rs
66:        ArgExtension::Sext => attrs.push(llvm::AttributeKind::SExt.create_attr(cx.llcx)),

[SpriteOvO]

@rustbot claim

I wanna give it a try.

[rustbot]

Error: Parsing assign command in comment failed: ...' claim' | error: expected end of command at >| ', I wanna '...

Please file an issue on GitHub at triagebot if there's a problem with this bot, or reach out on #t-infra on Zulip.

[topperc]

After staring at the code a bit longer. I think the RISC-V specific code only runs via adjust_for_foreign_abi which is called from fn_abi_adjust_for_abi when the ABI isn't rust. I see there's already an X86 specific change in fn_abi_adjust_for_abi. Do we need to add one for RISC-V there?

[workingjubilee]

After staring at the code a bit longer. I think the RISC-V specific code only runs via adjust_for_foreign_abi which is called from fn_abi_adjust_for_abi when the ABI isn't rust. I see there's already an X86 specific change in fn_abi_adjust_for_abi. Do we need to add one for RISC-V there?

Probably so!

Unless this is expected to be generally profitable, in which case we could give doing it on all platforms a go.

[michaelmaitland]

One option to fix is to add the extension in rustc_target/src/abi/call/riscv.rs like we do in rustc_target/src/abi/call/loongarch.rs::extend_integer_width.

Edit: it looks like we tried to but didn't?

[topperc]

One option to fix is to add the extension in rustc_target/src/abi/call/riscv.rs like we do in rustc_target/src/abi/call/loongarch.rs::extend_integer_width.

Edit: it looks like we tried to but didn't?

Wasn't the loongarch.rs code copied from riscv.rs?

The problem is that no target specific code is used for the Rust ABI.

[topperc]

After staring at the code a bit longer. I think the RISC-V specific code only runs via adjust_for_foreign_abi which is called from fn_abi_adjust_for_abi when the ABI isn't rust. I see there's already an X86 specific change in fn_abi_adjust_for_abi. Do we need to add one for RISC-V there?

Probably so!

Unless this is expected to be generally profitable, in which case we could give doing it on all platforms a go.

Is not generally profitable. It's a RISC-V-ism that was also copied to LoongArch. They are the only 2 in tree targets in LLVM that don't have i32 as a legal type.

[workingjubilee]

We should probably introduce a similar fn named something like adjust_for_rust_abi to make it obvious that you should apply architecture-specific modifiers to the Rust ABI as well for optimization purposes (instead of mere compliance).