This is an archive of the discontinued LLVM Phabricator instance.

[Triple] Add llvm::Triple::isSPARC{,32,64}
ClosedPublic

Authored by XiaodongLoong on Feb 22 2022, 7:41 PM.

Diff Detail

Event Timeline

XiaodongLoong created this revision.Feb 22 2022, 7:41 PM
XiaodongLoong requested review of this revision.Feb 22 2022, 7:41 PM
Herald added a project: Restricted Project. · View Herald TranscriptFeb 22 2022, 7:41 PM

Please sync past 3a1cb362370d223e09899d234726e15b52327b0e: the patch accidentally removes isDriverKit.

Please sync past 3a1cb362370d223e09899d234726e15b52327b0e: the patch accidentally removes isDriverKit.

@MaskRay Sorry, it is OK now. Thanks!

ro added a comment.Feb 23 2022, 6:58 AM

Looking at existing cases of comparisons with Triple::sparc and Triple::sparcv9, I guess we should have isSPARC32 and isSPARC64, too, like isMIPS32/isMIPS64 and isPPC32/isPPC64. When switching to isSPARC* from existing Triple::sparc* comparisons, one will have to be extra careful if they are meant to mean SPARC in general or 32-bit SPARC in particular. little-endian SPARC is another beast altogether: I didn't even know it existed beyond the ability of SPARCV9 CPUs to partially run in LE mode, so it will be hard to tell if LE SPARC is to be included. I guess only code review and testing by interested parties can tell.

llvm/include/llvm/ADT/Triple.h
828

I'd go for isSPARC instead: SPARC is an acronym, like MIPS, although LLVM is anything but consistent about the correct capitalization.

update code for review comments.
rebase code.

XiaodongLoong marked an inline comment as done.Feb 23 2022, 6:47 PM
XiaodongLoong added inline comments.
llvm/include/llvm/ADT/Triple.h
828

Thanks for the comment, I fixed it.

@ro
Since sparcv9 is Bi-endianness (https://en.wikipedia.org/wiki/Endianness#Bi-endianness), I add "sparcv9el" like on MIPS. If it is not OK, I can delete it.
Could I do the switching to isSPARC* from existing Triple::sparc* in another patch?

XiaodongLoong retitled this revision from [SPARC] Add isSparc for SPARC to [SPARC] Add isSPARC* in Triple for SPARC.Feb 23 2022, 7:01 PM
brad added a subscriber: brad.Feb 23 2022, 11:34 PM

Since sparcv9 is Bi-endianness (https://en.wikipedia.org/wiki/Endianness#Bi-endianness), I add "sparcv9el" like on MIPS. If it is not OK, I can delete it.

I don't think this makes sense. No one runs SPARCv9 with little endian.

XiaodongLoong updated this revision to Diff 411017.EditedFeb 23 2022, 11:50 PM

@brad delete little endian SPARC V9

ro added a comment.Feb 24 2022, 1:31 AM

I don't think this makes sense. No one runs SPARCv9 with little endian.

Not only that: this isn't a full fledged little-endian mode like mipsel. See "Oracle SPARC Architecture 2015", p.20:

3.3.1.2 Addressing Conventions
[...]
The Oracle SPARC Architecture also supports little-endian byte order for data accesses only: the
address of a quadword, doubleword, word, or halfword is the address of its least significant byte.
Increasing the address means increasing the significance of the data unit being accessed.
ro added a comment.Feb 24 2022, 3:49 AM

Could I do the switching to isSPARC* from existing Triple::sparc* in another patch?

Given the evaluation and testing requirements I'd mentioned, I guess it's better to start using isSPARC* for new or modified code right now, but handle existing comparisons to Triple::sparc* separately. I don't think that's your duty, though. You've done way enough already. Thanks.

ro added inline comments.Feb 24 2022, 3:52 AM
llvm/include/llvm/ADT/Triple.h
828

I think all those clang-format reformatting suggestions should be ignored: while they might be appropriate for new code, the new methods would be formatted differently from every other one in this file.

format new code

ro added inline comments.Feb 24 2022, 4:31 AM
llvm/include/llvm/ADT/Triple.h
833

That's the opposite of what I suggested: please follow existing formatting.

836

Same here.

XiaodongLoong added inline comments.Feb 24 2022, 4:49 AM
llvm/include/llvm/ADT/Triple.h
833

I am so sorry. I misunderstood your words. I format back.

836

Done.

ro accepted this revision.Feb 25 2022, 1:23 AM

LGTM

This revision is now accepted and ready to land.Feb 25 2022, 1:23 AM
MaskRay accepted this revision.Feb 25 2022, 1:48 PM
MaskRay updated this revision to Diff 411517.Feb 25 2022, 1:50 PM
MaskRay retitled this revision from [SPARC] Add isSPARC* in Triple for SPARC to [Triple] Add llvm::Triple::isSPARC{,32,64}.

clang-format

update subject

This revision was landed with ongoing or failed builds.Feb 25 2022, 1:50 PM
This revision was automatically updated to reflect the committed changes.