This is an archive of the discontinued LLVM Phabricator instance.

Add sparc_le architecture (little-endian 32-bit Sparc).
ClosedPublic

Authored by dougk on Mar 31 2015, 11:43 AM.

Diff Detail

Repository
rL LLVM

Event Timeline

dougk updated this revision to Diff 22990.Mar 31 2015, 11:43 AM
dougk retitled this revision from to Add sparc_le architecture (little-endian 32-bit Sparc)..
dougk updated this object.
dougk edited the test plan for this revision. (Show Details)
dougk added a subscriber: Unknown Object (MLST).

In the description, you meant to say copied from Sparc V8, right? (as this has created a sparcv8 variant, not a v9 variant).

(and of course some test cases)

lib/Target/Sparc/MCTargetDesc/SparcMCTargetDesc.cpp
54 ↗(On Diff #22990)

Why a new copy of this? Shouldn't it just use createSparcMCAsmInfo?

lib/Target/Sparc/SparcTargetMachine.cpp
29 ↗(On Diff #22990)

Perhaps less confusing to just have a bool isLittleEndian instead of a char passed in here?

dougk updated this revision to Diff 23000.Mar 31 2015, 1:16 PM
dougk updated this object.

Changed per jyknight review comments.

I realize this is going to seem like a terrible nit to pick on this patch, but I'd really prefer not to have the _ in sparc_le... none of the rest of the explicit little endian triples use an underscore.

A few additional comments inline. I'll want to take another look when you've fixed these if you don't mind.

Thanks!

-eric

lib/Target/Sparc/SparcSubtarget.cpp
50 ↗(On Diff #23000)

Two things here:

a) Comments are sentences
b) IsLittleEndian can reside on the TargetMachine, it doesn't need to be propagated down to the subtarget necessarily since we're already propagating down the TargetMachine. You can just store and query a reference to the TargetMachine if you need to.

lib/Target/Sparc/SparcTargetMachine.cpp
66 ↗(On Diff #23000)

If you're using sparc*le as your triple (e.g. sparcle, sparcv8le, sparcv9le) you don't need to pass isLittleEndian down to things, you can just use the target triple string and check for the architecture.

dougk updated this revision to Diff 23418.Apr 8 2015, 8:10 AM
dougk updated this object.

Changes requested by echristo.

Can you split out the triple part of the patch and add a test for it in unittests please? It'll reduce the size of the patch here a bit. You'll also want to clang-format this patch.

-eric

factored out the changes to Triple as requested - http://reviews.llvm.org/D9263

lib/Target/Sparc/MCTargetDesc/SparcMCTargetDesc.cpp
54 ↗(On Diff #22990)

Done

lib/Target/Sparc/SparcTargetMachine.cpp
29 ↗(On Diff #22990)

Done

Hmm... Can you rebase this one then (I've approved the Triple patch)?

Thanks!

-eric

Hmm... Can you rebase this one then (I've approved the Triple patch)?

Thanks!

-eric

dougk updated this revision to Diff 24489.Apr 27 2015, 11:13 AM

rebased assuming 'sparcel' has been added to Triple.

Few inline comments. Should be ok otherwise.

Thanks!

-eric

lib/Target/Sparc/MCTargetDesc/SparcAsmBackend.cpp
224 ↗(On Diff #24489)

No need for a temporary here?

lib/Target/Sparc/SparcTargetMachine.cpp
63 ↗(On Diff #24489)

clang-format?

135 ↗(On Diff #24489)

Ditto?

echristo accepted this revision.Apr 27 2015, 11:22 AM
echristo added a reviewer: echristo.
This revision is now accepted and ready to land.Apr 27 2015, 11:22 AM

will upload new patch with clang-format-diff. also explained the use of temp vars for '=='

lib/Target/Sparc/MCTargetDesc/SparcAsmBackend.cpp
224 ↗(On Diff #24489)

It was cargo-cultism based on the example above.
Without the temps you have to convert the LHS to a StringRef by hand because getName() returns char* and == would compare pointers. See if you like it.

dougk updated this revision to Diff 24495.Apr 27 2015, 12:03 PM
dougk edited edge metadata.

fixed minor nits

LGTM. Thanks!

-eric

This revision was automatically updated to reflect the committed changes.