This is an archive of the discontinued LLVM Phabricator instance.

[Support] Add CSKY target parser and attributes parser
ClosedPublic

Authored by zixuan-wu on Feb 15 2022, 11:49 PM.

Details

Summary

Construct LLVM Support module about CSKY target parser and attribute parser. It refers implementation of GNU binutils and GCC.

Now we only support CSKY 800 series cpus and newer cpus in the future undering CSKYv2 ABI specification. There are 11 archs including ck801, ck802, ck803, ck803s, ck804, ck805, ck807, ck810, ck810v, ck860, ck860v.

Every arch has base extensions, the cpus of that arch family have more extended extensions than base extensions. We need specify extended extensions for every cpu. Every extension has its enum value, name and related llvm feature string with +/-. Every enum value represents a bit of uint64_t integer.

Diff Detail

Event Timeline

zixuan-wu created this revision.Feb 15 2022, 11:49 PM
zixuan-wu requested review of this revision.Feb 15 2022, 11:49 PM
Herald added a project: Restricted Project. · View Herald TranscriptFeb 15 2022, 11:49 PM

I won't be able to check all the details but some general comments.

You say this is based on the GNU behaviour, and that's probably fine, but do you have any links to CSKY specifications? Don't need one for each CPU but if there's an architecture manual and an ELF ABI document I would link those in the commit message.

Clearly this is based on the Arm target parser and while I'd like to say hey let's dedupe Arm/AArch64/AVR(?)/CSKY/X86 that's an issue bigger than this change. You've kept to the same style so it won't make it any harder to do so in the future.

I think you might need to do some more work before this is needed, but soon you'll be able to add a csky entry to clang/test/Misc/target-invalid-cpu-note.c.

llvm/lib/Support/CSKYAttributeParser.cpp
148

I assume that here there's no need for "None" or "Error" because you'd only be reading the hard FP mode if you already got hardfp from fpuABI.

llvm/lib/Support/CSKYTargetParser.cpp
64

I'm tempted to say you don't need this, but if you've seen the compiler warn about it being missing then it's not doing any harm.

140

I don't know that this makes a difference but the ARMTargetParser.cpp includes are done as "llvm/support/ARMTargetParser.def".
(maybe not all of them)

llvm/unittests/Support/CSKYAttributeParserTest.cpp
32

I would put the length and bytesize on their own line so that it's more clear that each one is 4 bytes. (If I understood correctly from the comment)

Also you could:

OS << ....
       << ...more stuff;

Skip the OS on each subsequent line.

65

Is there a need to test that CSKYAttributeParser::fpuABI(unsigned tag) returns "Error" for some value?

Maybe you already did this and I missed it.

74

At first glance I'd expect to see tests for the other attributes:

Error dspVersion(unsigned tag);
Error vdspVersion(unsigned tag);
Error fpuVersion(unsigned tag);
Error fpuABI(unsigned tag);
Error fpuRounding(unsigned tag);
Error fpuDenormal(unsigned tag);
Error fpuException(unsigned tag);
Error fpuHardFP(unsigned tag);

Is there a reason they are not included here?

Yes, it's better to dedupe code in follow-up patches.

llvm/lib/Support/CSKYAttributeParser.cpp
148

Sorry. I don't know what exactly you mean. Make fpuHardFP return other type instead of Error?

DavidSpickett added inline comments.Feb 22 2022, 2:12 AM
llvm/lib/Support/CSKYAttributeParser.cpp
148

I wondered if you needed a way to signal that the hard FP modes were unrecognised or none were enabled.

Likely you're going to check this later though so different code will handle that situation. When you are checking that 2 objects are compatible.

zixuan-wu updated this revision to Diff 410745.EditedFeb 23 2022, 1:34 AM

Address comments.

DavidSpickett added inline comments.Feb 23 2022, 7:28 AM
llvm/unittests/Support/CSKYAttributeParserTest.cpp
36

Thanks for adding the comments, I would just do as clang-format asks here. With the comments to denote each section it makes sense.

DavidSpickett added inline comments.Feb 23 2022, 7:49 AM
llvm/unittests/Support/CSKYAttributeParserTest.cpp
110–111

As this attribute is a string, so it doesn't make sense to exhaustively check every arch name here since all this is doing is pulling a string not checking what it's value is.

Correct? (same for cpu test below)

124

Does this need to check the "Error" case?

Same for the others that have use an "Error" string.

194

Add a test for the hardFP unknown error.

llvm/unittests/Support/CSKYTargetParserTest.cpp
89

Seems like llvm updated the copy of gtest since I refactored the Arm tests. It's letting you have 154 tests in one block, used to max out at 50.

In which case this can just be CSKYCPUTests.

1045

What is the purpose of this? Seems like it would be covered by CSKYCPUTestsPart1.

zixuan-wu added inline comments.Feb 24 2022, 11:44 PM
llvm/unittests/Support/CSKYAttributeParserTest.cpp
110–111

So, how to generate a random string?

llvm/unittests/Support/CSKYTargetParserTest.cpp
1045

Right. I think it is covered by CSKYCPUTestsPart1 and TargetParserTest::CSKYArchExtFeature below.

Address comments.

DavidSpickett added inline comments.Feb 25 2022, 1:24 AM
llvm/unittests/Support/CSKYAttributeParserTest.cpp
110–111

I don't mean that you should use a random string here.

I'm just checking that I understand the test's goal here. Since the function being tested does not validate the CPU name, there is no need to check it with every possible name. Something else will check the content of the string later.

Or in other words don't change anything I think I convinced myself how it works. :)

DavidSpickett accepted this revision.Feb 25 2022, 1:26 AM

LGTM with the redundant tests removed.

llvm/unittests/Support/CSKYTargetParserTest.cpp
1045

Meaning they can be removed?

This revision is now accepted and ready to land.Feb 25 2022, 1:26 AM
zixuan-wu updated this revision to Diff 411721.Feb 27 2022, 6:39 PM
This revision was landed with ongoing or failed builds.Feb 27 2022, 7:35 PM
This revision was automatically updated to reflect the committed changes.