Page MenuHomePhabricator

[RFC] [LLVM] Allocatable Global Register Variables for ARM
AbandonedPublic

Authored by carwil on Dec 21 2018, 7:45 AM.

Details

Reviewers
None
Summary

Provides the backend support for using r5-r11 as globally scoped register variables. This will prevent them from being used by the register allocator, and from being saved to the stack.
It should be noted that this does change the behaviour of the old -ffixed-r9 option. This option will now also prevent its register from being spilled to the stack.
Clang patch: https://reviews.llvm.org/D56003

Diff Detail

Event Timeline

carwil created this revision.Dec 21 2018, 7:45 AM
carwil updated this revision to Diff 181354.Jan 11 2019, 12:45 PM
carwil edited the summary of this revision. (Show Details)
  • Re-implementation using subtarget features over metadata.
  • Reduction of register range to R5-R11.
  • Additional checks to fix issues with spilling registers for stack alignment.

Please repost this patch with llvm-commits CC'ed.

(I plan to review in detail this week.)

carwil updated this revision to Diff 181754.Jan 15 2019, 2:35 AM
carwil set the repository for this revision to rL LLVM.

Added llvm-commits.

amilendra edited reviewers, added: amilendra; removed: amilendra_arm.Jan 18 2019, 5:08 AM
carwil updated this revision to Diff 183115.EditedJan 23 2019, 9:26 AM

Added more test coverage, notably those to catch issues with using reserved registers for alignment or making up Thumb reg deficit.

carwil updated this revision to Diff 185275.Feb 5 2019, 3:57 AM
carwil edited the summary of this revision. (Show Details)

Fixed an oversight where R6 could be used as the base pointer, even with ffixed-r6 specified.

carwil added a subscriber: eli.friedman.EditedFeb 15 2019, 2:23 AM

Hi @efriedma, are you still happy to review this?

Sorry about the delay.

It looks like there's a conflict between reserving r7/r11 and the use of r7/r11 as a frame pointer. (See useR7AsFramePointer.) Similarly, there's a conflict between reserving r6 and its use as a base pointer. It's okay if some combinations print an error, but we shouldn't silently miscompile.

lib/Target/ARM/ARMSubtarget.h
722

Can we handle this in initSubtargetFeatures instead, like we do for rwpi? It's sort of confusing to follow.

test/CodeGen/ARM/reg-alloc-no-alignment.ll
16

Newline.

Not at all, thank you your comments.

I believe we have handled the r7/11 (FP) case when using clang (clang patch: https://reviews.llvm.org/D56003). We throw a warning here, as even specifying -fomit-frame-pointer can't seem to *guarantee* that the compiler won't touch them. We could restrict its usage entirely, but on thumb at least, that wouldn't leave you with much choice.

I think the R6/BP problem was resolved in my last diff by disabling the BP (lib/Target/ARM/ARMBaseRegisterInfo.cpp, line 416), when r6 has been reserved.

That said, if you used LLVM directly, without clang... Yes, we will need (a) warning(s) in the backend as well.

Yes, we should error in the backend, regardless of what clang does, in case some other frontend is missing the appropriate diagnostics.

Dynamically allocating memory on the stack requires a frame pointer... although I guess with -fomit-frame-pointer, we could try to choose a register that isn't reserved instead of unconditionally using the platform default.

lib/Target/ARM/ARMBaseRegisterInfo.cpp
376

In the case where we have stack realignment and VLAs, what happens? IIRC we don't have any code to generate an alternate sequence (although I guess that would be theoretically possible to dynamically allocate stack slots that would otherwise require realignment). Do we print an error message?

carwil abandoned this revision.Oct 11 2019, 2:36 AM
carwil marked an inline comment as done.
lib/Target/ARM/ARMSubtarget.h
722

I'm not quite sure what you mean. This is just a convenience function for checking which registers have been reserved. We're not actually setting the reservations here, that's handled by the ARM.td/reserve-rN rule(s).

r9 could be reserved either with ffixed-r9/reserve-r9 or with -frwpi, the second case being handled in initTargetSubFeatures.
For the other GPRs we only have the ffixed options, so there is no need.

Am I misunderstanding?