-
Notifications
You must be signed in to change notification settings - Fork 12.3k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[RISCV] Allow -mcmodel= to accept large for RV64 #107817
Conversation
@llvm/pr-subscribers-clang @llvm/pr-subscribers-clang-driver Author: Jim Lin (tclin914) ChangesFull diff: https://github.com/llvm/llvm-project/pull/107817.diff 2 Files Affected:
diff --git a/clang/lib/Driver/ToolChains/CommonArgs.cpp b/clang/lib/Driver/ToolChains/CommonArgs.cpp
index 0601016c3b14b8..f0e1b59076c738 100644
--- a/clang/lib/Driver/ToolChains/CommonArgs.cpp
+++ b/clang/lib/Driver/ToolChains/CommonArgs.cpp
@@ -2906,7 +2906,8 @@ void tools::addMCModel(const Driver &D, const llvm::opt::ArgList &Args,
CM = "small";
else if (CM == "medany")
CM = "medium";
- Ok = CM == "small" || CM == "medium";
+ Ok = CM == "small" || CM == "medium" ||
+ (CM == "large" && Triple.isRISCV64());
} else if (Triple.getArch() == llvm::Triple::x86_64) {
Ok = llvm::is_contained({"small", "kernel", "medium", "large", "tiny"},
CM);
diff --git a/clang/test/Driver/riscv-mcmodel.c b/clang/test/Driver/riscv-mcmodel.c
new file mode 100644
index 00000000000000..2482672d625fe3
--- /dev/null
+++ b/clang/test/Driver/riscv-mcmodel.c
@@ -0,0 +1,20 @@
+// RUN: %clang --target=riscv32 -### -c -mcmodel=small %s 2>&1 | FileCheck --check-prefix=SMALL %s
+// RUN: %clang --target=riscv64 -### -c -mcmodel=small %s 2>&1 | FileCheck --check-prefix=SMALL %s
+
+// RUN: %clang --target=riscv32 -### -c -mcmodel=medlow %s 2>&1 | FileCheck --check-prefix=SMALL %s
+// RUN: %clang --target=riscv64 -### -c -mcmodel=medlow %s 2>&1 | FileCheck --check-prefix=SMALL %s
+
+// RUN: %clang --target=riscv32 -### -c -mcmodel=medium %s 2>&1 | FileCheck --check-prefix=MEDIUM %s
+// RUN: %clang --target=riscv64 -### -c -mcmodel=medium %s 2>&1 | FileCheck --check-prefix=MEDIUM %s
+
+// RUN: %clang --target=riscv32 -### -c -mcmodel=medany %s 2>&1 | FileCheck --check-prefix=MEDIUM %s
+// RUN: %clang --target=riscv64 -### -c -mcmodel=medany %s 2>&1 | FileCheck --check-prefix=MEDIUM %s
+
+// RUN: not %clang --target=riscv32 -### -c -mcmodel=large %s 2>&1 | FileCheck --check-prefix=ERR-LARGE %s
+// RUN: %clang --target=riscv64 -### -c -mcmodel=large %s 2>&1 | FileCheck --check-prefix=LARGE %s
+
+// SMALL: "-mcmodel=small"
+// MEDIUM: "-mcmodel=medium"
+// LARGE: "-mcmodel=large"
+
+// ERR-LARGE: error: unsupported argument 'large' to option '-mcmodel=' for target 'riscv32'
|
What's the status of backend support for the large code model? That would presumably be a prerequisite to landing this. |
It was just merged: #70308. |
I expected I was out of date, thanks for the link. The psABI says the large code model is not compatible with PIC - this presumably still applies to LLVM's implementation? If so, I think we need to add an unsupported error message in this case. |
I think we want to set a define for this as well, for consistency with medany and medlow (as tested in test/Preprocessor/riscv-cmodel.c). I submitted a trivial PR to riscv-c-api-doc to add this riscv-non-isa/riscv-c-api-doc#86 which I'd hope lands quickly. EDIT: This change also deserves an entry in clang/docs/ReleaseNotes.rst IMHO. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What I am missing is:
- adjustments in
clang/lib/Basic/Targets/RISCV.cpp
to emit the macro__riscv_cmodel_large
- new tests in
clang/test/Preprocessor/riscv-cmodel.c
Related PRs:
This is a pre-commit test for #107817
7bc57f0
to
cb5e946
Compare
A follow-up PR #108131 for |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, on the basis the preprocessor changes are in a different PR.
This is a pre-commit test for llvm#107817
No description provided.