Message ID | 20230605110724.21391-24-andy.chiu@sifive.com (mailing list archive) |
---|---|
State | Accepted |
Commit | e4bb020f3dbb83912eb6799a9d4bb79da4fd77ec |
Headers | show |
Series | riscv: Add vector ISA support | expand |
Context | Check | Description |
---|---|---|
conchuod/cover_letter | success | Series has a cover letter |
conchuod/tree_selection | success | Guessed tree name to be for-next at HEAD 90502d51ab90 |
conchuod/fixes_present | success | Fixes tag not required for -next series |
conchuod/maintainers_pattern | success | MAINTAINERS pattern errors before the patch: 6 and now 6 |
conchuod/verify_signedoff | success | Signed-off-by tag matches author and committer |
conchuod/kdoc | success | Errors and warnings before: 0 this patch: 0 |
conchuod/build_rv64_clang_allmodconfig | success | Errors and warnings before: 8 this patch: 8 |
conchuod/module_param | success | Was 0 now: 0 |
conchuod/build_rv64_gcc_allmodconfig | success | Errors and warnings before: 8 this patch: 8 |
conchuod/build_rv32_defconfig | success | Build OK |
conchuod/dtb_warn_rv64 | success | Errors and warnings before: 3 this patch: 3 |
conchuod/header_inline | success | No static functions without inline keyword in header files |
conchuod/checkpatch | success | total: 0 errors, 0 warnings, 0 checks, 20 lines checked |
conchuod/build_rv64_nommu_k210_defconfig | success | Build OK |
conchuod/verify_fixes | success | No Fixes tag |
conchuod/build_rv64_nommu_virt_defconfig | success | Build OK |
Hi Andy, On Mon, Jun 05, 2023 at 11:07:20AM +0000, Andy Chiu wrote: > +config AS_HAS_OPTION_ARCH > + # https://reviews.llvm.org/D123515 > + def_bool y > + depends on $(as-instr, .option arch$(comma) +m) > + depends on !$(as-instr, .option arch$(comma) -i) With tip-of-tree clang (llvm-project commit 85a8e5c3e0586e85), I'm seeing AS_HAS_OPTION_ARCH be set to n. It's the second "depends on" that makes it be set to n, so apparently clang started accepting ".option arch -i". What was your intent here for checking that ".option arch -i" is not supported? I'd think that just the first "depends on" would be sufficient. - Eric
On Sat, 20 Jan 2024 17:13:41 PST (-0800), ebiggers@kernel.org wrote: > Hi Andy, > > On Mon, Jun 05, 2023 at 11:07:20AM +0000, Andy Chiu wrote: >> +config AS_HAS_OPTION_ARCH >> + # https://reviews.llvm.org/D123515 >> + def_bool y >> + depends on $(as-instr, .option arch$(comma) +m) >> + depends on !$(as-instr, .option arch$(comma) -i) > > With tip-of-tree clang (llvm-project commit 85a8e5c3e0586e85), I'm seeing > AS_HAS_OPTION_ARCH be set to n. It's the second "depends on" that makes it be > set to n, so apparently clang started accepting ".option arch -i". What was > your intent here for checking that ".option arch -i" is not supported? I'd > think that just the first "depends on" would be sufficient. I'm not sure what Andy's rationale was, but de3a913df6e ("RISC-V: Clarify the behavior of .option arch directive.") in binutils-gdb stopped accepting `.option arch, -i` along with fixing a handful of other oddities in our `.option arch` handling. If that's all this is testing for then we should probably add some sort of version check for old binutils (or maybe just ignore it, looks like it was a bugfix and the old version was never released). +Nelson, as he probably knows better than I do. That said: what does LLVM do if you ask it to turn the "I" base ISA off? I'd argue there's no instructions left at that point...
On Sun, Jan 21, 2024 at 10:55 AM Palmer Dabbelt <palmer@dabbelt.com> wrote: > > On Sat, 20 Jan 2024 17:13:41 PST (-0800), ebiggers@kernel.org wrote: > > Hi Andy, > > > > On Mon, Jun 05, 2023 at 11:07:20AM +0000, Andy Chiu wrote: > >> +config AS_HAS_OPTION_ARCH > >> + # https://reviews.llvm.org/D123515 > >> + def_bool y > >> + depends on $(as-instr, .option arch$(comma) +m) > >> + depends on !$(as-instr, .option arch$(comma) -i) > > > > With tip-of-tree clang (llvm-project commit 85a8e5c3e0586e85), I'm seeing > > AS_HAS_OPTION_ARCH be set to n. It's the second "depends on" that makes it be > > set to n, so apparently clang started accepting ".option arch -i". What was > > your intent here for checking that ".option arch -i" is not supported? I'd > > think that just the first "depends on" would be sufficient. The reason why I added the second check is because clang and gcc only return an assembler warning when it does not support ".option arch" at all. No errors were reported. So, it ended up passing the first condition check for old toolchains that shouldn't be passing. > > I'm not sure what Andy's rationale was, but de3a913df6e ("RISC-V: > Clarify the behavior of .option arch directive.") in binutils-gdb > stopped accepting `.option arch, -i` along with fixing a handful of > other oddities in our `.option arch` handling. > > If that's all this is testing for then we should probably add some sort > of version check for old binutils (or maybe just ignore it, looks like > it was a bugfix and the old version was never released). > > +Nelson, as he probably knows better than I do. > > That said: what does LLVM do if you ask it to turn the "I" base ISA off? > I'd argue there's no instructions left at that point... Maybe what we really should do is to upgrade the condition check to a one liner shell script and grep if "Warning" is being printed. Sadly this warning is not failing the compilation with -Werror. I can try forming a patch on this if it feels alright to people. Thanks, Andy
On Sun, Jan 21, 2024 at 10:32:59PM +0800, Andy Chiu wrote: > > Maybe what we really should do is to upgrade the condition check to a > one liner shell script and grep if "Warning" is being printed. Sadly > this warning is not failing the compilation with -Werror. > > I can try forming a patch on this if it feels alright to people. What about -Wa,--fatal-warnings ? - Eric
On Sun, Jan 21, 2024 at 10:10:09AM -0800, Eric Biggers wrote: > On Sun, Jan 21, 2024 at 10:32:59PM +0800, Andy Chiu wrote: > > > > Maybe what we really should do is to upgrade the condition check to a > > one liner shell script and grep if "Warning" is being printed. Sadly > > this warning is not failing the compilation with -Werror. > > > > I can try forming a patch on this if it feels alright to people. > > What about -Wa,--fatal-warnings ? I suspect that would work, the following diff appears to work for me with a version of clang that does and does not support '.option arch', (although I am not sure if adding -Wa,--fatal-warnings will have any other consequences): diff --git a/arch/riscv/Kconfig b/arch/riscv/Kconfig index bffbd869a068..e3142ce531a0 100644 --- a/arch/riscv/Kconfig +++ b/arch/riscv/Kconfig @@ -315,7 +315,6 @@ config AS_HAS_OPTION_ARCH # https://reviews.llvm.org/D123515 def_bool y depends on $(as-instr, .option arch$(comma) +m) - depends on !$(as-instr, .option arch$(comma) -i) source "arch/riscv/Kconfig.socs" source "arch/riscv/Kconfig.errata" diff --git a/scripts/Kconfig.include b/scripts/Kconfig.include index 5a84b6443875..3ee8ecfb8c04 100644 --- a/scripts/Kconfig.include +++ b/scripts/Kconfig.include @@ -33,7 +33,7 @@ ld-option = $(success,$(LD) -v $(1)) # $(as-instr,<instr>) # Return y if the assembler supports <instr>, n otherwise -as-instr = $(success,printf "%b\n" "$(1)" | $(CC) $(CLANG_FLAGS) -c -x assembler-with-cpp -o /dev/null -) +as-instr = $(success,printf "%b\n" "$(1)" | $(CC) $(CLANG_FLAGS) -Wa$(comma)--fatal-warnings -c -x assembler-with-cpp -o /dev/null -) # check if $(CC) and $(LD) exist $(error-if,$(failure,command -v $(CC)),C compiler '$(CC)' not found)
On Mon, Jan 22, 2024 at 03:29:18PM -0700, Nathan Chancellor wrote: > On Sun, Jan 21, 2024 at 10:10:09AM -0800, Eric Biggers wrote: > > On Sun, Jan 21, 2024 at 10:32:59PM +0800, Andy Chiu wrote: > > > > > > Maybe what we really should do is to upgrade the condition check to a > > > one liner shell script and grep if "Warning" is being printed. Sadly > > > this warning is not failing the compilation with -Werror. > > > > > > I can try forming a patch on this if it feels alright to people. > > > > What about -Wa,--fatal-warnings ? > > I suspect that would work, the following diff appears to work for me > with a version of clang that does and does not support '.option arch', > (although I am not sure if adding -Wa,--fatal-warnings will have any > other consequences): > > diff --git a/arch/riscv/Kconfig b/arch/riscv/Kconfig > index bffbd869a068..e3142ce531a0 100644 > --- a/arch/riscv/Kconfig > +++ b/arch/riscv/Kconfig > @@ -315,7 +315,6 @@ config AS_HAS_OPTION_ARCH > # https://reviews.llvm.org/D123515 > def_bool y > depends on $(as-instr, .option arch$(comma) +m) > - depends on !$(as-instr, .option arch$(comma) -i) > > source "arch/riscv/Kconfig.socs" > source "arch/riscv/Kconfig.errata" > diff --git a/scripts/Kconfig.include b/scripts/Kconfig.include > index 5a84b6443875..3ee8ecfb8c04 100644 > --- a/scripts/Kconfig.include > +++ b/scripts/Kconfig.include > @@ -33,7 +33,7 @@ ld-option = $(success,$(LD) -v $(1)) > > # $(as-instr,<instr>) > # Return y if the assembler supports <instr>, n otherwise > -as-instr = $(success,printf "%b\n" "$(1)" | $(CC) $(CLANG_FLAGS) -c -x assembler-with-cpp -o /dev/null -) > +as-instr = $(success,printf "%b\n" "$(1)" | $(CC) $(CLANG_FLAGS) -Wa$(comma)--fatal-warnings -c -x assembler-with-cpp -o /dev/null -) > > # check if $(CC) and $(LD) exist > $(error-if,$(failure,command -v $(CC)),C compiler '$(CC)' not found) It looks good to me as long as none of the as-instr users turn out to have any expected warnings. Maybe send the change to scripts/Kconfig.include as a separate patch? - Eric
diff --git a/arch/riscv/Kconfig b/arch/riscv/Kconfig index 348c0fa1fc8c..1019b519d590 100644 --- a/arch/riscv/Kconfig +++ b/arch/riscv/Kconfig @@ -262,6 +262,12 @@ config RISCV_DMA_NONCOHERENT config AS_HAS_INSN def_bool $(as-instr,.insn r 51$(comma) 0$(comma) 0$(comma) t0$(comma) t0$(comma) zero) +config AS_HAS_OPTION_ARCH + # https://reviews.llvm.org/D123515 + def_bool y + depends on $(as-instr, .option arch$(comma) +m) + depends on !$(as-instr, .option arch$(comma) -i) + source "arch/riscv/Kconfig.socs" source "arch/riscv/Kconfig.errata" @@ -466,7 +472,7 @@ config TOOLCHAIN_HAS_ZBB depends on !64BIT || $(cc-option,-mabi=lp64 -march=rv64ima_zbb) depends on !32BIT || $(cc-option,-mabi=ilp32 -march=rv32ima_zbb) depends on LLD_VERSION >= 150000 || LD_VERSION >= 23900 - depends on AS_IS_GNU + depends on AS_HAS_OPTION_ARCH config RISCV_ISA_ZBB bool "Zbb extension support for bit manipulation instructions"