This is an archive of the discontinued LLVM Phabricator instance.

Separate CSRs into a subset handled via explicit copies and a subset handled by prologue/epilogue (AArch64)
ClosedPublic

Authored by manmanren on Dec 8 2015, 11:04 AM.

Diff Detail

Event Timeline

manmanren updated this revision to Diff 42199.Dec 8 2015, 11:04 AM
manmanren retitled this revision from to Separate CSRs into a subset handled via explicit copies and a subset handled by prologue/epilogue (AArch64).
manmanren updated this object.
hfinkel added a subscriber: hfinkel.Dec 9 2015, 4:37 AM
hfinkel added inline comments.
lib/Target/AArch64/AArch64ISelLowering.cpp
10055

Do you need to emit any kind of CFI pseudo-instructions for these so that unwinding works?

manmanren added inline comments.Dec 9 2015, 2:16 PM
lib/Target/AArch64/AArch64ISelLowering.cpp
10055

This is a good point. I think we should but I am not so sure.

Maybe Nick is a better person to answer the question on Darwin platform.

Specific to C++-style TLS access functions, they should be nounwind. Throwing an exception from the initializer of a thread-local variable is defined to immediately result in a call to std::terminate(); there’s no legal way for unwinding to actually leave such a function. (quote from John McCall) :]

hfinkel added inline comments.Dec 9 2015, 2:31 PM
lib/Target/AArch64/AArch64ISelLowering.cpp
10055

In that case, please add a comment about this. If someone goes to generalize this later (or port it to another target), it should be obvious that this is something they'll need to think about.

manmanren updated this revision to Diff 42539.Dec 11 2015, 10:21 AM

Addressing Hal's comment. Patch is also updated to work with the target independent portion (http://reviews.llvm.org/D15340).

qcolombet edited edge metadata.Dec 15 2015, 11:22 AM

Hi Manman,

Thanks for your patience.

Here are a few comments.

Cheers,
-Quentin

lib/Target/AArch64/AArch64ISelLowering.cpp
3276

Why is this using a pointer type instead of an i64?

3277

No need for {}

3280

Ditto.

3283

Was this clang-formated?
The layout looks funny to me.

10059

Could you assert that the MachineFunction have the nounwind attribute?

lib/Target/AArch64/AArch64ISelLowering.h
392

I think we should change the hook to take a MachineFunction.
That way we could check that the function does have the nounwind attribute and if it does not, we can say this is not supported.

lib/Target/AArch64/AArch64MachineFunctionInfo.h
75

Add a comment for that field.

lib/Target/AArch64/AArch64RegisterInfo.cpp
61

Isn’t this redundant with the isSplitCSR check?
I mean, if we want to support more calling convention, we should minimize the places we have to patch to do so.

test/CodeGen/AArch64/cxx-tlscc.ll
50

Nice! :)

manmanren updated this revision to Diff 42893.Dec 15 2015, 12:33 PM
manmanren edited edge metadata.

Thanks Quentin for reviewing the patch!

Addressing his comments with the updated patch.

manmanren marked 7 inline comments as done.Dec 15 2015, 12:37 PM

Addressing review comments,

Cheers,
Manman

lib/Target/AArch64/AArch64RegisterInfo.cpp
61

If we are going to support other calling conventions with split CSR, it will be listed here with "else if", so we know which corresponding ViaCopy_SaveList to use. Similarly in getCalleeSavedRegs above.

qcolombet accepted this revision.Dec 16 2015, 10:31 AM
qcolombet edited edge metadata.

Hi Manman,

LGTM.

Thanks,
-Quentin

This revision is now accepted and ready to land.Dec 16 2015, 10:31 AM
This revision was automatically updated to reflect the committed changes.