This is an archive of the discontinued LLVM Phabricator instance.

[RISCV] Add support for -ffixed-xX flags
ClosedPublic

Authored by simoncook on Sep 4 2019, 9:38 AM.

Details

Summary

This adds support for reserving GPRs such that the compiler will not choose a register for register allocation. The implementation follows the same design as for AArch64; each reserved register becomes a target feature and used for getting the reserved registers for a given MachineFunction. The backend checks that it does not need to use a reserved register for any ABI purpose; if it does a relevant error is generated.

Diff Detail

Event Timeline

simoncook created this revision.Sep 4 2019, 9:38 AM
Herald added projects: Restricted Project, Restricted Project. · View Herald TranscriptSep 4 2019, 9:38 AM

I don't quite understand all the details of this patch. I understand reserving registers that the compiler would otherwise be using as general-purpose registers.

But what do we do about using registers within the calling convention when someone says they should be reserved against compiler use? I think you're saying that GCC ignores that they should be fixed, and uses them anyway - it seems like that would cause hard-to-diagnose errors, for instance if a user requests fixed x8 but requests the use of frame pointers, it seems like that should be an error, and yet here it might not be? I think the clang-y approach would be to say "you can't fix x8, we're going to use it anyway", at which point the "fix x8" option becomes pointless.

llvm/test/CodeGen/RISCV/reserved-regs.ll
72

These tests aren't going to test what you think they are, or at least aren't going to fail when you hope they are, as the allocator will probably choose a0 for these registers, even if other registers are available (it uses the GPRs in a specific order, starting with the first free one, which will usually be a0).

jrtc27 added inline comments.Sep 4 2019, 12:18 PM
llvm/test/CodeGen/RISCV/reserved-regs.ll
72

This is approximately what llvm/test/CodeGen/AArch64/arm64-platform-reg.ll is doing, and the pint is that @var is big enough that the register allocator will use every register it can so the order shouldn't matter.

As for reserving registers that have a specific use in the calling convention, AArch64 allows you to use them but won't let you make function calls (llvm/test/CodeGen/AArch64/arm64-reserved-arg-reg-call-error.ll). I don't know what it does about function arguments though, it probably assumes they're in the ABI registers on entry but then doesn't allocate/clobber those registers within the function? I'm not quite sure what that's useful for though. GCC has this to say about the flag:

Treat the register named reg as a fixed register; generated code should never refer to it (except perhaps as a stack pointer, frame pointer or in some other fixed role).

So it's pretty vague about what can happen if you try and reserve a register normally used for other things...

simoncook planned changes to this revision.Sep 5 2019, 2:05 AM

Thanks for the feedback. I will improve the test so it more reliably tests what it intends to.

With regards to behaviour surrounding things such as argument registers, before submitting I checked what the riscv port of GCC does, and it matches this behaviour. If a register is used for arg passing/return values then the option is accepted but silently ignored (at least the register is still used for passing arguments, I haven't confirmed for other regalloc purposes).

I agree that this has the opportunity for allowing users to think they've reserved a register but it is still used. I will look at something more to what you've described AArch64 does in LLVM, and also check that there isn't also a bug on the GCC side, if so I'll get that fixed too.

For added context, I have gone and double-checked with GCC's implementation both for AArch64 and RISC-V and for registers used by the calling convention the compiler will still use them for argument passing and return values, but otherwise won't use it for any temporaries/register allocation purposes, which does have the side effect of confusing behaviour unless carefully documented.

I can implement errors for using calling convention registers when there are functions that take arguments, but this would be an explicit deviation in behaviour between the two compilers. I presume we would want to do that anyway because it's safer/more clear?

simoncook updated this revision to Diff 219068.Sep 6 2019, 5:15 AM
simoncook edited the summary of this revision. (Show Details)

Update based on initial feedback/going down the providing error route.

Unlike AArch64, which provides an error if a function tries to call a function with arguments and any of the argument passing registers are used, I've gone down a more detailed route. I've added tests for any place where any ABI register will be modified, and produce an error indicating what part of the ABI requires this register.

Nice, I like this new approach! One naming nit, but overall I think this is much better than the first version of the patch.

LGTM but I would like @luismarques to take a look too.

llvm/lib/Target/RISCV/RISCVSubtarget.h
49

Nit: Can this have a name that reveals it is for registers that are reserved by the user, rather than generally reserved?

We would like to differentiate ra being reserved because of the calling convention, vs ra being reserved because a user asked for -ffixed-x1.

simoncook updated this revision to Diff 220876.Sep 19 2019, 9:03 AM

Update to reflect comments about the fact registers are explicitly reserved. In addition to @lenary 's suggested change, I renamed isReservedReg to note the check that we are checking if its a user provided reservation.

simoncook updated this revision to Diff 223005.Oct 3 2019, 6:56 AM
simoncook added a reviewer: luismarques.

Rebase on top of tree, add @luismarques as reviewer

luismarques accepted this revision.Oct 13 2019, 9:00 AM

Overall LGTM. Caveats:

  • Address the issues in the inline comments;
  • Shouldn't the TLS lowering also complain when -ffixed-x4 is used?
  • Is there a way to ensure we don't forget to check any such reserved reg uses? I'm not quite confident we haven't overlooked anything.
  • (Remember to check for the -ffixed-xX flags when implementing the callee-saved regs via libcalls (D62686), etc.)

Apologies for the delayed review.

clang/include/clang/Driver/Options.td
2204–2206

Given the expansion of the flags here, the AArch64 driver should probably detect and reject the flags -ffixed-x[8,16-17,19,29-31], to preserve the old behavior where passing those flags would be an error and to ensure that erroneous flags are not silently accepted.

llvm/lib/Target/RISCV/RISCVISelLowering.cpp
2410

clang-format indicates another formatting style here.

llvm/lib/Target/RISCV/RISCVSubtarget.cpp
53

This includes more than the x0 - x31 registers. If the intent is to only allow reserving the GPRs then this should be tightened.

llvm/lib/Target/RISCV/RISCVSubtarget.h
97

Consider adding a bounds checking assert.

This revision is now accepted and ready to land.Oct 13 2019, 9:00 AM

Note, D68862 is in-progress at the moment, which is related to this patch.

simoncook marked 2 inline comments as done.Oct 14 2019, 3:18 AM
simoncook added inline comments.
clang/include/clang/Driver/Options.td
2204–2206

With this patch, for AArch64 using -ffixed-x8 for example will produce a warning that the flag is unused, similar to any other architecture trying to use these flags; I'll look at a follow-up patch that explicitly produces errors.

llvm/lib/Target/RISCV/RISCVSubtarget.cpp
53

For now this only covers GPRs, but going forward I was going to follow up with one for floating point, since these should also be reservable if we match all of GCCs behaviour, where every register should be marked as reserved.

asb added a comment.Oct 14 2019, 9:35 AM

Note, D68862 is in-progress at the moment, which is related to this patch.

Indeed - Simon, could you please go through that patch and ensure that the implementation here is aligned to it (or indeed, feed back on that patch if you feel they should be doing something differently). Thanks!

luismarques added inline comments.Oct 17 2019, 6:28 AM
llvm/lib/Target/RISCV/RISCVSubtarget.h
96

This should take a Register argument instead.

In D67185#1708177, @asb wrote:

Note, D68862 is in-progress at the moment, which is related to this patch.

Indeed - Simon, could you please go through that patch and ensure that the implementation here is aligned to it (or indeed, feed back on that patch if you feel they should be doing something differently). Thanks!

It looks like that patch is following the same route we are, using target features to implement this, both inspired by AArch64. There were some questions about using SubtargetFeatures for these kind of purposes and should these be function attributes instead. Given we’re already using features for things like relaxation, I’m inclined to stick with target features rather than split these between two different methodologies, especially since we have the AArch64 precedence in this case.

This revision was automatically updated to reflect the committed changes.

@simoncook: your commit doesn't include handling the case of TLS lowering when -ffixed-x4 is used.

@simoncook: your commit doesn't include handling the case of TLS lowering when -ffixed-x4 is used.

I looked at this, and did start writing the patch that covers the use of TP/X4 for TLS lowering, but didn't push it because it doesn't match the rest of the the cases I error on here. As it stands, what landed produces errors whenever a reserved register is modified, but not when they are read (so in for example the argument case you can use incoming arguments, but not lower calls to functions that need arguments.

For TLS I believe we would only be consuming X4, so erroring here would be inconsistent. I don't necessarily think producing an error whenever the compiler wishes to read a particular register is useful, for inlineasm there's no way to verify the validity of reading a particular register, but I don't think we can express that to the backend well anyway. I can work on a follow-up error/warn on all the reads case if we think that's of value, but as far as the intended purpose of stopping the compiler clobbering reserved registers, I don't think there's anything needed in that case.