This is an archive of the discontinued LLVM Phabricator instance.

[AArch64] Support adding X[8-15,18] registers as CSRs.
ClosedPublic

Authored by trong on Sep 17 2018, 6:33 PM.

Details

Summary

Specifying X[8-15,18] registers as callee-saved is used to support
CONFIG_ARM64_LSE_ATOMICS in Linux kernel. As part of this patch we:

  • use custom CSR list/mask when user specifies custom CSRs
  • update Machine Register Info's list of CSRs with additional custom CSRs in

LowerCall and LowerFormalArguments.

Diff Detail

Event Timeline

trong created this revision.Sep 17 2018, 6:33 PM
lib/Target/AArch64/AArch64ISelLowering.cpp
4054–4058

This pattern is repeated, a lot.

Maybe you could make a single function like:

void UpdateMaskCustomCall (const MachineFunction& MF, uint32_t** Mask) {
  ...
}
...
{
   uint32_t* Mask = ...;
   MachineFunction &MF = ...;
   UpdateMaskCustomCall(MF, &Mask);
}

?

lib/Target/AArch64/AArch64.td
109

Since x18 can now be call-saved, any thoughts to how llvm should handle x18 being both call-saved and reserved (above line 102)? Maybe a test can be added for this case?

trong updated this revision to Diff 166048.Sep 18 2018, 4:29 PM
  • Added UpdateMaskCustomCall() util function to AArch64ISelLowering.
  • Added test for case when x18 is specified as both reserved and call-saved.
trong added inline comments.Sep 18 2018, 4:44 PM
lib/Target/AArch64/AArch64.td
109

I think -fcall-saved-x18 should have no effect in presence of -ffixed-x18. If x18 is reserved, callee shouldn't allocate x18. So there shouldn't be a reason to save it. Added a test case.

lib/Target/AArch64/AArch64ISelLowering.cpp
4054–4058

MachineFunction will have to be passed non-const to make MF.allocateRegMask() call. So the signature would look something like:

void UpdateMaskCustomCall (MachineFunction &MF, uint32_t** Mask)

AArch64RegisterInfo is probably not a good place to change the state of a MachineFunction, so added a function to AArch64ISelLowering to do this. I left AArch64CallLowering as is since it only gets the custom mask once.

lib/Target/AArch64/AArch64CallLowering.cpp
343

This looks very very similar to UpdateMaskCustomCall. Is there a way to convert a uint32_t[] to a SmallVector<MCPhysReg, 32>? That might help cut down on code duplication further.

390–394

UpdateMaskCustomCall?

lib/Target/AArch64/AArch64ISelLowering.cpp
3110

Don't we already have a reference to the Subtarget on the previous line? auto TRI = Subtarget.getRegisterInfo();?

7783–7784

It seems like all invocations of UpdateMaskCustomCall have this pattern:

if (x) UpdateMaskCustomCall(...);

where x is the same condition. Can the condition be moved in to UpdateMaskCustomCall and then UpdateMaskCustomCall called unconditionally, only actually doing anything if (x)?

lib/Target/AArch64/AArch64ISelLowering.h
550 ↗(On Diff #166048)

Is there somewhere this can go so that it can continue to be used from lib/Target/AArch64/AArch64ISelLowering.cpp but additionally lib/Target/AArch64/AArch64CallLowering.cpp? See above comment.

It seems like Subtarget is accessible through the provided MF via MF.getSubtarget<AArch64Subtarget>().

lib/Target/AArch64/AArch64RegisterInfo.cpp
82–83

Can std::copy be used here?

85–89

Can std::copy_if be used here?

92

no need for empty return.

149–151

Just trying to understand what SubReg is in this case? I would think we only need to iterate over the number of registers once, so I don't understand the inner for loop. I've read the comment above the declaration of MCSubRegIterator, but I guess I don't understand what a subreg is in context of aarch64.

154

grep did not turn up matches for this method. Typo, or was a function you added then renamed?

160

Not an expert on the LLVM coding style, but no need for empty returns in void returning functions.

trong updated this revision to Diff 166215.Sep 19 2018, 8:00 PM
trong marked 4 inline comments as done.
  • Added helper functions to AArch64RegisterInfo to update CSR list/mask with custom calling convention during instruction selection.
trong marked an inline comment as done.Sep 19 2018, 8:03 PM
trong added inline comments.
lib/Target/AArch64/AArch64CallLowering.cpp
343

Since CSR masks and CSR lists have different format, we have to deal with them differently.

lib/Target/AArch64/AArch64ISelLowering.cpp
3110

right

7783–7784

Even though hasCustomCallingConv() checks are repetitive, I'd like to keep the check outside of UpdateMaskCustomCall for readability of instruction selection code.

lib/Target/AArch64/AArch64ISelLowering.h
550 ↗(On Diff #166048)

Subtarget only exposes target information, so I don't think Subtarget is good place to make changes to MF. Added helper functions to AArch64RegisterInfo (not ideal either) so that they could be used by both isel implementations.

lib/Target/AArch64/AArch64RegisterInfo.cpp
82–83

We rely on last value of the list being 0. We'd have to look for the end of the list first to use std::copy, which is not as nice as looping through just once.

85–89

We don't have direct access to the copied container.

92

right

149–151

E.g. w0 (32-bit) is a subregister of x0 (64-bit)
CSR masks account for all subregisters:
https://github.com/llvm-mirror/llvm/blob/master/include/llvm/CodeGen/TargetRegisterInfo.h#L471
So while looping over X registers we also need to set the bit for the corresponding W register.

154

Yes, it's a typo, should be getCallPreservedMask.

nickdesaulniers accepted this revision.Sep 20 2018, 9:46 AM

Thanks Tri, this looks good to me!

This revision is now accepted and ready to land.Sep 20 2018, 9:46 AM
This revision was automatically updated to reflect the committed changes.