PowerPC inline ASM: Fix scalar floats being in the wrong vector lane on little endian

[beetrees]

64-bit PowerPC supports both big and little endian, however registers are always big endian. As a consequence of this, the order of vector lanes is reversed on little endian; however scalar f32 and f64 are always stored in the actual (big-endian) lane 0. This PR fixes the LLVM ASM fixup to take that into account.

Ping target maintainers: @daltenty @gilamn5tr @amy-kwan @Gelbpunkt @famfo @neuschaefer

[rustbot]

r? @JonathanBrouwer

rustbot has assigned @JonathanBrouwer.
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 75 candidates
• Random selection from 19 candidates

[JonathanBrouwer]

@rustbot reroll

[Gelbpunkt]

64-bit PowerPC supports both big and little endian, however registers are always big endian. As a consequence of this, the order of vector lanes is reversed on little endian; however scalar f32 and f64 are always stored in the actual (big-endian) lane 0. This PR fixes the LLVM ASM fixup to take that into account.

To nitpick a bit here, the registers themselves have no endianness. However, the lane numbering in mnemonics is always "big-endian", i.e. counted from left to right, see e.g. the Vector/SIMD Multimedia Extension Technology Programming Environments Manual, section 1.2.2.1, "Byte Ordering".

If we look at the Power ISA™ Version 3.0C specification, we see:

As shown in Figure 97, the elements in Vector Registers are numbered; the high-order (or most significant) byte element is numbered 0 and the low-order (or least significant) byte element is numbered 15

and if we look for example at xscvdpsp in the same document, it will always operate on doubleword element 0 and place the result in word elements 0 and 1 (i.e. the highest-order elements).

LLVM is aware that these instructions are lane-sensitive, see llvm/llvm-project@77e34f1. However, the numbering used in the commit description differs from the ISA specification:

These two put the result into lane 0 (BE) or 3 (LE) regardless of the input

because LLVM's notion of lane numbering appears to refer to the logical vector lanes rather than the ISA's architectural element numbers.

Therefore the change is correct.

See for example the added test on real hardware without the fix:

pub fn f64_to_f32(x: f64) -> f32 {
    let res;
    unsafe {
        core::arch::asm!("xscvdpsp {}, {}", out(vsreg) res, in(vsreg) x, options(pure, nostack, nomem));
    };
    res
}

fn main() {
    println!("{}", f64_to_f32(std::f64::consts::PI));
}

currently on powerpc64-unknown-linux-musl:

aelin@algol ~> lscpu | head -n 6
Architecture:                            ppc64
CPU op-mode(s):                          32-bit, 64-bit
Byte Order:                              Big Endian
CPU(s):                                  40
On-line CPU(s) list:                     0-39
Model name:                              POWER8 (architected), altivec supported
aelin@algol ~> rustc -Ctarget-feature=+vsx test.rs && ./test
warning: unstable feature specified for `-Ctarget-feature`: `vsx`
  |
  = note: this feature is not stably supported; its behavior can change in the future

warning: 1 warning emitted

3.1415927

currently on powerpc64le-unknown-linux-musl:

aelin@fiora ~> lscpu | head -n 5
Architecture:                            ppc64le
Byte Order:                              Little Endian
CPU(s):                                  32
On-line CPU(s) list:                     0-31
Model name:                              POWER9 (raw), altivec supported
aelin@fiora ~> rustc test.rs && ./test
0

Thank you for the fix!

[folkertdev]

Some nits. I do really like the use of check-prefix, we should probably use that more, it's really useful in the LLVM repo.

Probably

r? @Amanieu

Also technically this is a breaking change then? Even though it is also a bug fix.

View changes since this review

[beetrees]

Also technically this is a breaking change then? Even though it is also a bug fix.

Technically yes, however since the previous behaviour didn't work at all with hardware instructions expecting a scalar (and any uses that just used hardware vector instructions instead will also work with the fixed behaviour) it seems unlikely any current usage will actually be broken by this bug fix.

[folkertdev]

Right. I'll nominate for T-lang because it is technically a breaking change, but I don't see how this could be controversial at all and be anything but an immediate FCP (if that, maybe because this is really a bug fix we can even forego it).

[traviscross]

Makes sense to me. I propose to waive the 10-day final comment period (with all checkboxes).

(I'm setting aside here whether it needs an FCP. It's safer to just FCP it than to risk us not getting to this in the meeting and thereby leaving it sit.)

@rfcbot fcp merge lang

[rust-rfcbot]

@traviscross has proposed to merge this. The next step is review by the rest of the tagged team members:

• @joshtriplett
• @nikomatsakis
• @scottmcm
• @tmandry
• @traviscross

No concerns currently listed.

Once a majority of reviewers approve (and at most 2 approvals are outstanding), this will enter its final comment period. If you spot a major issue that hasn't been raised at any point in this process, please speak up!

See this document for info about what commands tagged team members can give me.

[nikomatsakis]

@rfcbot reviewed

[rust-rfcbot]

🔔 This is now entering its final comment period, as per the review above. 🔔

[scottmcm]

I haven't dug into exactly the change, but it sounds like a bugfix to me -- if there's no way that it was ever possible or desired intended to put them into the current spot, I'd probably do this without an FCP.

@rfcbot reviewed

[joshtriplett]

Registering that this should not be a lang matter; it seems like an architectural bugfix. We need to find a process by which we can stop having these matters come to lang.

But for today:

@rfcbot reviewed

[tmandry]

@rfcbot reviewed

[beetrees]

(Just noticed I missed adding power8 to --check-cfg. I've added #![deny(unexpected_cfgs)] to avoid that happening in future.)