This revision was automatically updated to reflect the committed changes.
Closed by commit rL330880: [RISCV] More validations on the input value of
-march= (authored by apazos, committed by ).
Herald added a subscriber: llvm-commits.
Changed prior to commit:
asb accepted this revision.
asb added a comment.
This revision is now accepted and ready to land.
Looks good to me - thanks!
https://reviews.llvm.org/D45284
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
apazos updated this revision to Diff 143674.
apazos added a comment.
Hi Alex, the refactoring will be simple and can be done later when we need it,
all the pieces are already parsed (type, name, major, minor) and are in
strings, we will only need to convert to the preferred type (enum, int,
asb added a comment.
In https://reviews.llvm.org/D45284#1074282, @apazos wrote:
> Hi Alex, it seems the table expects these extensions in a canonical order
> too: all x extensions, followed by all s extensions, and then all sx
> extensions.
>
> I can make the change, no problem. I have also
apazos added a comment.
Hi Alex, it seems the table expects these extensions in a canonical order too:
all x extensions, followed by all s extensions, and then all sx extensions.
I can make the change, no problem. I have also coded other error situations
described below.
But f I cannot push a
asb added a comment.
Herald added a subscriber: edward-jones.
This is looking great, the only remaining code comment I have is that
getExtensionFeatures needs a comment describing it.
The remaining issue I have is more of a spec issue - do canonical ordering
requirements apply to extension
apazos added a comment.
Addressed the latest review comments.
Added TODOs for validations we cannot do now.
https://reviews.llvm.org/D45284
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
apazos updated this revision to Diff 142716.
apazos edited the summary of this revision.
https://reviews.llvm.org/D45284
Files:
include/clang/Basic/DiagnosticDriverKinds.td
lib/Driver/ToolChains/Arch/RISCV.cpp
test/Driver/riscv-arch.c
Index: test/Driver/riscv-arch.c
apazos added inline comments.
Comment at: lib/Driver/ToolChains/Arch/RISCV.cpp:50
+
+static void getExtensionVersion(StringRef In, std::string ) {
+ auto I = In.begin();
asb wrote:
> You should probably document the limitation that this doesn't currently parse
asb added inline comments.
Comment at: lib/Driver/ToolChains/Arch/RISCV.cpp:50
+
+static void getExtensionVersion(StringRef In, std::string ) {
+ auto I = In.begin();
You should probably document the limitation that this doesn't currently parse
minor versions
apazos updated this revision to Diff 142502.
apazos added a comment.
Fixed failure in release mode
https://reviews.llvm.org/D45284
Files:
include/clang/Basic/DiagnosticDriverKinds.td
lib/Driver/ToolChains/Arch/RISCV.cpp
test/Driver/riscv-arch.c
Index: test/Driver/riscv-arch.c
apazos updated this revision to Diff 142459.
apazos added a comment.
- Simplified hasOtherExtensions() now that we clarified non-standard extensions
are separated by '_'. We just need to find the first occurrence of the prefixes.
- updated error message, removed "unsupported" from some messages.
apazos updated this revision to Diff 142279.
apazos added a comment.
Updated error messages and fixed getExtensionVersion
https://reviews.llvm.org/D45284
Files:
include/clang/Basic/DiagnosticDriverKinds.td
lib/Driver/ToolChains/Arch/RISCV.cpp
test/Driver/riscv-arch.c
Index:
asb added a comment.
I've added a few comments on tweaking the error messages based on your tests.
Comment at: test/Driver/riscv-arch.c:157
+// RV32-STR: error: invalid arch name 'rv32',
+// RV32-STR: string must begin with rv32 or rv64
+
But the given string
apazos updated this revision to Diff 141717.
apazos edited the summary of this revision.
apazos added a comment.
Herald added a subscriber: zzheng.
Updated code according to the ISA string rules that have been clarified.
https://reviews.llvm.org/D45284
Files:
asb added a comment.
Based on Andrew's response (thanks Kito for sending the query) it looks like
GCC accepting lowercase only is intentional, and we should follow that. In
which case, it might be an improvement to reject uppercase letters in the ISA
string with a message saying that only
kito-cheng added a comment.
Long time ago, GCC also accept upper case too, but I have no idea why Andrew
change that? I guess one possible reason is because multi-lib?
[1]
https://github.com/riscv/riscv-gcc/commit/6531a11f03ec3a95cd8b9033daeab0ebf23b5472
https://reviews.llvm.org/D45284
asb added inline comments.
Comment at: test/Driver/riscv-arch.c:151
+// RUN: -fsyntax-only 2>&1 | FileCheck -check-prefix=RV32-LETTER %s
+// RV32-LETTER: error: invalid arch name 'rv32e',
+// RV32-LETTER: first letter should be 'e', 'i' or 'g'
But rv32e is a
apazos updated this revision to Diff 141050.
apazos added a comment.
fixed test merged line
https://reviews.llvm.org/D45284
Files:
include/clang/Basic/DiagnosticDriverKinds.td
lib/Driver/ToolChains/Arch/RISCV.cpp
test/Driver/riscv-arch.c
Index: test/Driver/riscv-arch.c
apazos updated this revision to Diff 141048.
apazos added a comment.
test fix
https://reviews.llvm.org/D45284
Files:
include/clang/Basic/DiagnosticDriverKinds.td
lib/Driver/ToolChains/Arch/RISCV.cpp
test/Driver/riscv-arch.c
Index: test/Driver/riscv-arch.c
apazos updated this revision to Diff 141043.
apazos added a comment.
updated test case
https://reviews.llvm.org/D45284
Files:
include/clang/Basic/DiagnosticDriverKinds.td
lib/Driver/ToolChains/Arch/RISCV.cpp
test/Driver/riscv-arch.c
Index: test/Driver/riscv-arch.c
apazos created this revision.
apazos added reviewers: asb, kito-cheng.
Herald added subscribers: shiva0217, niosHD, sabuasal, jordy.potman.lists,
simoncook, johnrusso, rbar.
- Updated diagnostic messages for invalid/unsupported march combinations.
- Parsing X, SX and S extensions.
22 matches
Mail list logo
