[PATCH] D45284: [RISCV] More validations on the input value of -march=

[Ana Pazos via Phabricator via cfe-commits]

apazos added inline comments.


================
Comment at: lib/Driver/ToolChains/Arch/RISCV.cpp:50
+
+static void getExtensionVersion(StringRef In, std::string &Version) {
+  auto I = In.begin();
----------------
asb wrote:
> You should probably document the limitation that this doesn't currently parse minor versions e.g. i2p0.
Correct, it is parsing the  major version for each standard extension. Will make note of how to parse minor version.


================
Comment at: lib/Driver/ToolChains/Arch/RISCV.cpp:144-146
+      // Currently LLVM does not support 'e'.
+      D.Diag(diag::err_drv_invalid_riscv_arch_name)
+        << MArch << "unsupported standard user-level extension 'e'";
----------------
asb wrote:
> This could be tightened up by also rejected rv64e as invalid.
OK, will add an error message for the invalid combo 'rv64' and 'e', though e is not supported yet for rv32.


================
Comment at: lib/Driver/ToolChains/Arch/RISCV.cpp:201-202
+      if (StdExtsItr == StdExtsEnd) {
+        size_t Pos;
+        if (hasExtension(StdExts, std::string(1, c), Pos)) {
+          D.Diag(diag::err_drv_invalid_riscv_ext_arch_name)
----------------
asb wrote:
> I'd suggest either just using StringRef::contains and getting rid of hasExtension, or adding a doc comment to hasExtension to explain its semantics.
> 
> It might also be worth adding a comment to explain why you want to check the extension is present in the StdExts string (e.g. We have reached the end of the StdExts string. Either the current extension was given outside of the canonical order (in which case issue an error), or else no canonical ordering is defined meaning no error should be generated'.
When we reach here, either c contains a valid extension but it was not given in
canonical order or it is an invalid extension. The code that follows was checking for the former, while the check for the latter happens in the switch default statement right below. But we can handle both here and print the appropriate messages, and leave the the check in switch statement just error out if LLVM does not support the extension. Yes, we can also get rid of hasExtension, it is not used any other place anymore.



================
Comment at: lib/Driver/ToolChains/Arch/RISCV.cpp:211
       // Move to next char to prevent repeated letter.
       ++StdExtsItr;
 
----------------
asb wrote:
> Won't this now iterate StdExtsItr past StdExtsEnd if StdExtsItr == StdExtsEnd but the hasExtension call is false?
At this point, if StdExtsItr is StdExtsEnd, the code will error out in the switch case default statement. It means you found an invalid extension. Otherwise it will return due to invalid canonical order check above.

I willl move both error conditions to the same place to make the logic clearer.


================
Comment at: lib/Driver/ToolChains/Arch/RISCV.cpp:265-267
     if (HasD && !HasF)
-      D.Diag(diag::err_drv_invalid_arch_name) << MArch;
+      D.Diag(diag::err_drv_invalid_riscv_arch_name) << MArch
+        << "d requires f extension to also be specified";
----------------
asb wrote:
> Add a TODO about other dependencies perhaps? e.g. ef and efd are invalid, and q requires rv64.
will make a note of the additional dependency checks.


https://reviews.llvm.org/D45284