Update configure #1735
Update configure #1735
Conversation
|
This will break riscv-gnu-toolchain with existing GCC release since we don't have |
OKay, I get it. Thanks. |
|
I guess we may adjust configure script and Makefile a little bit more to make it don't pass |
@TelGome, are you interested in working on that? |
Yes, and I will modify the code later. |
|
Hi @kito-cheng @cmuellner , the code has been updated. Could you please review it again when you have time? Thanks! |
configure.ac
Outdated
| ) | ||
|
|
||
| if test -n "$with_tune" && test "$with_tune" != "yes"; then |
There was a problem hiding this comment.
This is usually done like this: test "x$with_tune" != "xyes".
But here this might be a better fit: test "x$with_tune" != "x".
Makefile.in
Outdated
| @@ -35,7 +35,7 @@ endif | |||
|
|
|||
| WITH_ARCH ?= @WITH_ARCH@ | |||
| WITH_ABI ?= @WITH_ABI@ | |||
| WITH_TUNE ?= @WITH_TUNE@ | |||
| WITH_TUNE = @WITH_TUNE@ | |||
There was a problem hiding this comment.
Keep this unchanged, ?= means only assign that when that variable is undefined - that allow user to override that, although it still override-able for =, but keep it ?= is kind of more explicitly to say this is override-able
There was a problem hiding this comment.
If i keep WITH_TUNE ?= @WITH_TUNE@, no "WITH_TUNE=" appear in the Makefile.
There was a problem hiding this comment.
If i keep
WITH_TUNE ?= @WITH_TUNE@, no "WITH_TUNE=" appear in the Makefile.
Shouldn't it be set by the configure script as modified by this PR?
There was a problem hiding this comment.
What you said is completely correct, and I will push the code later. Thanks.
There was a problem hiding this comment.
Pull Request Overview
This PR updates the RISC-V --with-tune handling in configure.ac, the generated configure script, and Makefile.in so that the default tune is now “generic” (GCC’s default) rather than “rocket”.
- Change default
--with-tunehelp text and remove the hardcoded “rocket” default. - Add conditional logic to only set
WITH_TUNEwhen a non-yes value is provided. - Adjust
Makefile.into unconditionally assignWITH_TUNEfrom the generated configure variable.
Reviewed Changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated no comments.
| File | Description |
|---|---|
| configure.ac | Updated --with-tune help string, removed default action, added conditional WITH_TUNE logic, and simplified AC_SUBST. |
| configure | Updated usage text, removed old default assignment, and added new WITH_TUNE conditional block. |
| Makefile.in | Changed WITH_TUNE ?= @WITH_TUNE@ to WITH_TUNE = @WITH_TUNE@ for direct assignment. |
Comments suppressed due to low confidence (2)
configure.ac:97
- [nitpick] The help text 'defaults to GCC's default' is somewhat vague; consider stating 'defaults to generic' explicitly so users immediately see the actual value.
[AS_HELP_STRING([--with-tune=generic],
Makefile.in:38
- [nitpick] Switching from '?=' to '=' means environment variables can no longer override WITH_TUNE; if you still want to allow user overrides via the environment, consider using '?=' instead of '='.
WITH_TUNE = @WITH_TUNE@
Since the patch "RISC-V: Add generic tune as default" has been commited into the trunk, the configure should also been updated. Here is the link: https://gcc.gnu.org/git/?p=gcc.git;a=commitdiff;h=20f593018519fec1602dc39c08ba2e674a2d8a1c Signed-off-by: TelGome <93700071+TelGome@users.noreply.github.com>
| WITH_TUNE="--with-tune=$with_tune" | ||
| else | ||
| WITH_TUNE="" | ||
|
|
There was a problem hiding this comment.
This empty line is not needed.
Since the patch "RISC-V: Add generic tune as default" has been commited into the trunk, the configure should also been updated. Here is the link: https://gcc.gnu.org/git/?p=gcc.git;a=commitdiff;h=20f593018519fec1602dc39c08ba2e674a2d8a1c