Page MenuHomePhabricator

[AArch64] Refactor LSE support as feature separate from V8.1a support.
ClosedPublic

Authored by joelkevinjones on Nov 14 2016, 11:14 AM.

Details

Summary

This is preparation for ThunderX processors that have Large System Extension (LSE)
instructions, but not the other instructions introduced by V8.1a. This will mimic changes to GCC as described here: https://gcc.gnu.org/ml/gcc-patches/2015-06/msg00388.html

LSE instructions are: LD/ST<op>, CAS*, SWP

Diff Detail

Repository
rL LLVM

Event Timeline

joelkevinjones retitled this revision from to [AArch64] Refactor LSE support as feature separate from V8.1a support..
joelkevinjones updated this object.
t.p.northover added inline comments.
lib/Target/AArch64/AArch64.td
39 ↗(On Diff #77844)

Can we expand "LSE" in the description here? I still can't actually think what it could mean.

t.p.northover edited edge metadata.Nov 23 2016, 9:44 AM

So what is LSE, exactly?

rengolin added inline comments.Nov 23 2016, 9:46 AM
lib/Target/AArch64/AArch64.td
39 ↗(On Diff #77844)

Ditto.

joelkevinjones edited edge metadata.

Added requested explanation of what LSE is.

OK, I eventually managed to track it down to "large system extensions", I think we should include that somewhere. Probably alongside noting that it's the atomics because the link is decidedly non-obvious even with the expanded name.

rengolin edited edge metadata.Nov 25 2016, 8:59 AM

OK, I eventually managed to track it down to "large system extensions", I think we should include that somewhere. Probably alongside noting that it's the atomics because the link is decidedly non-obvious even with the expanded name.

I think the current description is better than if it just said "Large System Extension", as it actually tells you what it is about. :)

I don't mind as it is, or an additional comment just about the declaration. Everything else looks fine, too. Leaving to Tim to approve.

cheers,
--renato

I think the current description is better than if it just said "Large System Extension", as it actually tells you what it is about. :)

I agree, I was pretty disappointed about how opaque it turned out to be. I still think it's better to have both though. Unexplained acronyms are really annoying.

Tim.

joelkevinjones added inline comments.Nov 28 2016, 9:48 AM
lib/Target/AArch64/AArch64.td
39 ↗(On Diff #77844)

So line 39 should be:

"Enable ARMv8.1 atomic memory access Large System Extension (LSE) instructions">'

or:

"Enable ARMv8.1 Large System Extension (LSE) atomic memory access instructions">'

First, second, or something else?

t.p.northover accepted this revision.Nov 28 2016, 9:54 AM
t.p.northover edited edge metadata.
t.p.northover added inline comments.
lib/Target/AArch64/AArch64.td
39 ↗(On Diff #77844)

I vaguely prefer the second (and dark blue, really closer to navy but not quite; with yellow spots), but either would be fine. No need to reupload a patch, the rest looks fine to me.

This revision is now accepted and ready to land.Nov 28 2016, 9:54 AM
This revision was automatically updated to reflect the committed changes.