Page MenuHomePhabricator

[RISCV] Sign-extend 32-bit integer inline assembly operands on RV64I
Needs ReviewPublic

Authored by jrtc27 on Oct 18 2019, 7:18 PM.

Details

Reviewers
asb
Summary

GCC forces its in-register representation of 32-bit integers to be
sign-extended (ignoring whether it was a signed type in the source),
which gets exposed to inline assembly, commonly due to comparisons
against other sign-extended registers written to in the assembly, such
as in FreeBSD's atomic.h.

Since i32 is not legal for RV64I, we end up with a zero extension
(presumably because isZExtFree returns true and is preferred) for i32
loads consumed by inline assembly, deviating from GCC's exposed
behaviour. 64-bit MIPS (for which GCC also has the same in-register
representation of 32-bit integers) does not suffer from this bug because
it has a GPR32 register class and can use a normal non-extending load
(which is by default the sign-extending variant).

Event Timeline

jrtc27 created this revision.Oct 18 2019, 7:18 PM
Herald added a project: Restricted Project. · View Herald TranscriptOct 18 2019, 7:18 PM
jrtc27 added a comment.EditedOct 19 2019, 5:43 PM

So, I'm not sure whether this is actually something we want to make work or not. You can already end up with both inconsistencies between compilers and inconsistencies within a compiler for how 16-bit values get represented when passed as register operands, and the fact that we're going against the grain and making i32 an illegal type just makes that also apply to i32; it's nothing new, just more widespread. I wrote a bunch of stuff exploring how this is all already broken for i16 over on the corresponding FreeBSD revision that found this issue (https://reviews.freebsd.org/D22084#482766), but trying to support this sounds like opening a can of worms, and it seems likely that we will still end up with subtle differences, ones which might rear their heads only once there is sufficient inlining (and perhaps due to LTO), removing the sanitisation applied by the calling convention. Moreover, only guaranteeing GCC compatibility (and self-consistency) for i32 and iPTR (whatever that may be) across architectures seems a bit arbitrary. Yes, i32 is a bit special due to C's integer promotion rules, but it feels like a slippery slope towards standardising on i16 and i8 too. Yes, I went and wrote this patch, but having further explored the issue and thought about it, my inclination is that we should just tell people to not do this, and put in the explicit sign-extending or zero-extending casts they want in order to get an XLEN-sized value. Especially since there is not likely to be much RISC-V inline assembly out there at the moment that relies on this. What do others think? Perhaps Clang could gain more than the current warn_asm_mismatched_size_modifier (which is currently about whether the register you're getting due to your modifiers matches what your input is, e.g. if the input gets promoted to an i32 on AArch64 you should be using a W register with %w0 rather than %0; it's not warning about the promotion itself).

bsdjhb added a subscriber: bsdjhb.Oct 21 2019, 9:49 AM

My only thought is that the RISC-V spec seems to require sign-extending unsigned 32-bit ints when stored in 64-bit registers, so folks may very well assume this behavior in the future when writing inline assembly. The spec doesn't say anything about types other than 32-bits that I found, and the section from the 2.2 user spec (quoted below) is in italics (meaning it's commentary to explain that spec that technically isn't part of the spec itself IIRC which perhaps puts it in a grey area). For FreeBSD it seems a simple cast in the constraint is sufficient to resolve our immediate issue FWIW.

From section 4.2:

The compiler and calling convention maintain an invariant that all 32-bit values are held in a sign-extended format in 64-bit registers. Even 32-bit unsigned integers extend bit 31 into bits 63 through 32. Consequently, conversion between unsigned and signed 32-bit integers is a no-op, as is conversion from a signed 32-bit integer to a signed 64-bit integer. Existing 64-bit wide SLTU and unsigned branch compares still operate correctly on unsigned 32-bit integers under this invariant. Similarly, existing 64-bit wide logical operations on 32-bit sign-extended integers preserve the sign-extension property. A few new instructions (ADD[I]W/SUBW/SxxW) are required for addition and shifts to ensure reasonable performance for 32-bit values.

I think there are a few issues here (having read the freebsd ticket):

  1. How do we choose to do something reasonable?
  2. How do we tell users there are sharp corners here?

To provide opinionated answers, having had a chat with @luismarques before we saw the comment from @bsdjhb:

  1. We need a sensible default here. Because GCC doesn't define any behaviour around inline assembly (and doesn't expect to maintain compatibility between versions), we can choose to do something principled and stick to that. Given the specification wording, I think sign-extending makes sense (given the specification), but if we want to do something different for signed vs unsigned types, this will be much more complex (LLVM IR doesn't have signed/unsigned types). I do think looking at the RISC-V specification and forming a reasonable behaviour based off that (without looking at other implementations) is the correct approach in this case.
  1. I absolutely believe we need a warning if the operand type does not match the register size. This is important for users who want to avoid this sharp edge (users who don't care can turn off warnings). It doesn't need to be enabled by default, but warnings are a great way of encouraging developers to improve their code, and also help to point to potential code issues when switching compilers/versions which may have incompatibilities (as inline assembly does have, see comment about GCC in 1.). I would go so far as to say this warning should be cross-target, so other targets can benefit too. I do realise this could be hard to implement.

I was not aware of that aside in the spec, which does indeed suggest this specific case should sign-extend, and I guess gives us a reason why this isn't a slippery slope. However, even if this is the case, I still feel code shouldn't be relying on this, simply because it's unclear (and perhaps surprising to those who don't know this particular note from the spec). One might perfectly reasonably assume that both signed and unsigned i32 types (in the original source) are zero-extended, or (probably the default position of people) that the signed type is sign-extended and the unsigned type is zero-extended (which would require passing the extension information through via the asm call instruction's operands).

I was not aware of that aside in the spec, which does indeed suggest this specific case should sign-extend, and I guess gives us a reason why this isn't a slippery slope. However, even if this is the case, I still feel code shouldn't be relying on this, simply because it's unclear (and perhaps surprising to those who don't know this particular note from the spec). One might perfectly reasonably assume that both signed and unsigned i32 types (in the original source) are zero-extended, or (probably the default position of people) that the signed type is sign-extended and the unsigned type is zero-extended (which would require passing the extension information through via the asm call instruction's operands).

FWIW, even though I had ready this part of the spec previously, I didn't really notice it until I went back and re-read it while investigating this specific issue (I was actually hunting for an unsigned variant of lw.r when I found this section instead). I think having a warning makes sense as this behavior is certainly not common across platforms. I also find it odd that this comment is commentary in the spec rather than formal language in the spec as it seems to assert that this behavior is required of the compiler rather than optional. For FreeBSD's atomic.h I'm going to add explicit casts to force sign-extension which I think would also satisfy the proposed warning.

mhorne added a subscriber: mhorne.Oct 21 2019, 1:53 PM