[PATCH] D119917: [Support] Add CSKY target parser and attributes parser

[David Spickett via Phabricator via llvm-commits]

DavidSpickett added a comment.

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`.



================
Comment at: llvm/lib/Support/CSKYAttributeParser.cpp:147
+  return Error::success();
+}
----------------
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.


================
Comment at: llvm/lib/Support/CSKYTargetParser.cpp:63
+    llvm_unreachable("Unknown FPU Kind");
+    return false;
+  }
----------------
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.


================
Comment at: llvm/lib/Support/CSKYTargetParser.cpp:139
+                  DEFAULT_EXT)
+#include "../../include/llvm/Support/CSKYTargetParser.def"
+      .Default(CSKY::AEK_INVALID);
----------------
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)


================
Comment at: llvm/unittests/Support/CSKYAttributeParserTest.cpp:31
+    OS << (uint8_t)1 << (uint8_t)6 << (uint8_t)0 << (uint8_t)0 << (uint8_t)0;
+    OS << (uint8_t)Tag << (uint8_t)Value;
+  }
----------------
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.


================
Comment at: llvm/unittests/Support/CSKYAttributeParserTest.cpp:64
+      testAttribute(17, 3, CSKYAttrs::CSKY_FPU_ABI, CSKYAttrs::HARD));
+}
+
----------------
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.


================
Comment at: llvm/unittests/Support/CSKYAttributeParserTest.cpp:73
+      testAttribute(16, 3, CSKYAttrs::CSKY_FPU_VERSION, CSKYAttrs::FPU_VERSION_3));
+}
----------------
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?


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D119917/new/

https://reviews.llvm.org/D119917