Page MenuHomePhabricator

[RISCV] Set triple based on -march flag which can be deduced in more generic way
Needs ReviewPublic

Authored by zixuan-wu on Jul 14 2022, 7:32 PM.

Details

Summary

Now RISC-V the value provided to -march can determine whether to compile for 32- or 64-bit RISC-V irrespective of the target provided to the Clang driver. It was introduced at https://reviews.llvm.org/D54214 before.

So this patch tries to go further based on D54214 that -march can be deduced in more generic way without providing -march but -mcpu or -mabi. To make it more clear.

Before:
clang -target=riscv32-- -march=rv64gc, riscv32 will be replaced by riscv64 to avoid report error.
After:
clang -target=riscv32-- -mcpu=sifive-s21, riscv32 will also be replaced by riscv64 because sifive-s21' default march is rv64imac provided at RISCVTargetParser.def.

Diff Detail

Unit TestsFailed

TimeTest
130 msx64 debian > LLVM.CodeGen/AArch64::active_lane_mask.ll
Script: -- : 'RUN: at line 2'; /var/lib/buildkite-agent/builds/llvm-project/build/bin/llc -mtriple=aarch64-linux-gnu -mattr=+sve < /var/lib/buildkite-agent/builds/llvm-project/llvm/test/CodeGen/AArch64/active_lane_mask.ll | /var/lib/buildkite-agent/builds/llvm-project/build/bin/FileCheck /var/lib/buildkite-agent/builds/llvm-project/llvm/test/CodeGen/AArch64/active_lane_mask.ll
100 msx64 debian > LLVM.CodeGen/AArch64::fdiv-combine.ll
Script: -- : 'RUN: at line 2'; /var/lib/buildkite-agent/builds/llvm-project/build/bin/llc -mtriple=aarch64-unknown-unknown < /var/lib/buildkite-agent/builds/llvm-project/llvm/test/CodeGen/AArch64/fdiv-combine.ll | /var/lib/buildkite-agent/builds/llvm-project/build/bin/FileCheck /var/lib/buildkite-agent/builds/llvm-project/llvm/test/CodeGen/AArch64/fdiv-combine.ll
130 msx64 debian > LLVM.CodeGen/AArch64::sve-fixed-length-fp-select.ll
Script: -- : 'RUN: at line 2'; /var/lib/buildkite-agent/builds/llvm-project/build/bin/llc -aarch64-sve-vector-bits-min=256 < /var/lib/buildkite-agent/builds/llvm-project/llvm/test/CodeGen/AArch64/sve-fixed-length-fp-select.ll | /var/lib/buildkite-agent/builds/llvm-project/build/bin/FileCheck /var/lib/buildkite-agent/builds/llvm-project/llvm/test/CodeGen/AArch64/sve-fixed-length-fp-select.ll -check-prefixes=CHECK,VBITS_GE_256
150 msx64 debian > LLVM.CodeGen/AArch64::sve-fixed-length-int-select.ll
Script: -- : 'RUN: at line 2'; /var/lib/buildkite-agent/builds/llvm-project/build/bin/llc -aarch64-sve-vector-bits-min=256 < /var/lib/buildkite-agent/builds/llvm-project/llvm/test/CodeGen/AArch64/sve-fixed-length-int-select.ll | /var/lib/buildkite-agent/builds/llvm-project/build/bin/FileCheck /var/lib/buildkite-agent/builds/llvm-project/llvm/test/CodeGen/AArch64/sve-fixed-length-int-select.ll -check-prefixes=CHECK,VBITS_GE_256
110 msx64 debian > LLVM.CodeGen/AArch64::sve-gep.ll
Script: -- : 'RUN: at line 2'; /var/lib/buildkite-agent/builds/llvm-project/build/bin/llc -mtriple=aarch64-linux-gnu -mattr=+sve < /var/lib/buildkite-agent/builds/llvm-project/llvm/test/CodeGen/AArch64/sve-gep.ll | /var/lib/buildkite-agent/builds/llvm-project/build/bin/FileCheck /var/lib/buildkite-agent/builds/llvm-project/llvm/test/CodeGen/AArch64/sve-gep.ll
View Full Test Results (7 Failed)

Event Timeline

zixuan-wu created this revision.Jul 14 2022, 7:32 PM
Herald added a project: Restricted Project. · View Herald TranscriptJul 14 2022, 7:32 PM
zixuan-wu requested review of this revision.Jul 14 2022, 7:32 PM

Does GCC allow this or not? Because this strikes me as a bad idea at first sight…

Does GCC allow this or not? Because this strikes me as a bad idea at first sight…

GCC can deduce from -mcpu but not -mabi.

riscv64-unknown-linux-gnu-gcc a.c -mabi=ilp32
cc1: error: ABI requires '-march=rv32'

But I think it can be negotiated.

eopXD added inline comments.Jul 15 2022, 2:15 AM
clang/lib/Driver/Driver.cpp
659

Do we need to throw error (or warning) when these two (RVArch and ArchName) don't match?

zixuan-wu added inline comments.Jul 18 2022, 12:42 AM
clang/lib/Driver/Driver.cpp
659

It just overrides it as before to not report error when for example default triple is rv64 with user passing -march=rv32xxx

lenary resigned from this revision.Jul 18 2022, 9:04 AM
zixuan-wu updated this revision to Diff 445690.Jul 18 2022, 9:08 PM

Remove unnecessary include header.

reames added a subscriber: reames.Jul 21 2022, 7:32 PM

This was very briefly discussed at today's sync up call. We were running short on time, so we didn't get a chance to talk through it, but there did seem to be a consensus that discussion on the interface implications was needed. This should hopefully be on the agenda when we talk again in two weeks.

This was very briefly discussed at today's sync up call. We were running short on time, so we didn't get a chance to talk through it, but there did seem to be a consensus that discussion on the interface implications was needed. This should hopefully be on the agenda when we talk again in two weeks.

Okay. Anybody else please more comments here before next sync-up call if you have.

If you still want to pursue this, discussion belongs at https://github.com/riscv-non-isa/riscv-toolchain-conventions, not here, since it's an interface shared by Clang and GCC and the two should be consistent

Just realized the problem is trying to fixed the default value of -mabi=, currently clang -target riscv32-elf -march=rv64gc -mabi=lp64d/riscv32-elf-clang -march=rv64gc -mabi=lp64d is work, and match the behavior of GCC did, riscv32-elf-gcc -march=rv64gc -mabi=lp64d.

And this patch is trying to make following two command work: clang -target riscv32-elf -march=rv64gc/riscv32-elf-clang -march=rv64gc, specify -march and -target but no -mabi.

That is different story now, GCC isn't deduce the default abi from either target triple or abi, so if you invoke gcc with riscv32-elf-gcc -march=rv64gc or riscv64-elf-gcc -march=rv32gc, you will got error message like that: cc1: error: ABI requires '-march=rv32' or cc1: error: ABI requires '-march=rv64'.

So that's not compatible issue with GCC, that's sort of clang driver specify issue, and I don't have strong opinion on this.


As a GNU toolchain developer, I would say, we are not intend to change the behavior of default value of -mabi or -march, the consensus among RISC-V GNU toolchain maintainer is user should explicitly specify the -march and -mabi if you are not using default -march and -mabi.

Just realized the problem is trying to fixed the default value of -mabi=, currently clang -target riscv32-elf -march=rv64gc -mabi=lp64d/riscv32-elf-clang -march=rv64gc -mabi=lp64d is work, and match the behavior of GCC did, riscv32-elf-gcc -march=rv64gc -mabi=lp64d.

And this patch is trying to make following two command work: clang -target riscv32-elf -march=rv64gc/riscv32-elf-clang -march=rv64gc, specify -march and -target but no -mabi.

That is different story now, GCC isn't deduce the default abi from either target triple or abi, so if you invoke gcc with riscv32-elf-gcc -march=rv64gc or riscv64-elf-gcc -march=rv32gc, you will got error message like that: cc1: error: ABI requires '-march=rv32' or cc1: error: ABI requires '-march=rv64'.

So that's not compatible issue with GCC, that's sort of clang driver specify issue, and I don't have strong opinion on this.


As a GNU toolchain developer, I would say, we are not intend to change the behavior of default value of -mabi or -march, the consensus among RISC-V GNU toolchain maintainer is user should explicitly specify the -march and -mabi if you are not using default -march and -mabi.

@kito-cheng Thanks! We have noticed these differences between GNU and LLVM, and we have done some efforts to make both compatible.
I think we can discuss here: https://github.com/riscv-non-isa/riscv-toolchain-conventions/issues/20

Just realized the problem is trying to fixed the default value of -mabi=, currently clang -target riscv32-elf -march=rv64gc -mabi=lp64d/riscv32-elf-clang -march=rv64gc -mabi=lp64d is work, and match the behavior of GCC did, riscv32-elf-gcc -march=rv64gc -mabi=lp64d.

And this patch is trying to make following two command work: clang -target riscv32-elf -march=rv64gc/riscv32-elf-clang -march=rv64gc, specify -march and -target but no -mabi.

That is different story now, GCC isn't deduce the default abi from either target triple or abi, so if you invoke gcc with riscv32-elf-gcc -march=rv64gc or riscv64-elf-gcc -march=rv32gc, you will got error message like that: cc1: error: ABI requires '-march=rv32' or cc1: error: ABI requires '-march=rv64'.

So that's not compatible issue with GCC, that's sort of clang driver specify issue, and I don't have strong opinion on this.


As a GNU toolchain developer, I would say, we are not intend to change the behavior of default value of -mabi or -march, the consensus among RISC-V GNU toolchain maintainer is user should explicitly specify the -march and -mabi if you are not using default -march and -mabi.

This patch is not about default value of mabi. It just tries to deduce march when -march option is not provided explicitly.
It fixes the conflict between target triple and march about 32-bit/64-bit (riscv32 or riscv64) issue as before. Before this patch, this conflict only can be solved by specifying -march option explicitly.

asb added a comment.Aug 8 2022, 8:07 AM

@zixuan-wu we discussed this a bit in the last community sync call. One aspect that makes this patch a little hard to review is the lack of a clear patch description summarising the behaviour before the patch, the behaviour after the patch, and the motivation for the change. I appreciate that the time isn't very convenient in all timezones, but if you were able to attend the next call (18th August, same time) to discuss, that might really help agree a path forwards on this change.

zixuan-wu edited the summary of this revision. (Show Details)Aug 8 2022, 6:40 PM

@zixuan-wu we discussed this a bit in the last community sync call. One aspect that makes this patch a little hard to review is the lack of a clear patch description summarising the behaviour before the patch, the behaviour after the patch, and the motivation for the change. I appreciate that the time isn't very convenient in all timezones, but if you were able to attend the next call (18th August, same time) to discuss, that might really help agree a path forwards on this change.

I updated the description. I think it can be understood after see D54214.

jrtc27 added inline comments.Aug 8 2022, 6:45 PM
clang/test/Driver/riscv-arch.c
410

This one maybe makes sense to allow, though gets murky once you have cores that support writable XLEN as then the same CPU exists for both RV32 and RV64 (like how you can ask for say haswell i386 and x86_64) so I'm not entirely sure...

414

IMO this kind of thing should remain an error, you're asking for an ABI that doesn't exist for the requested architecture

zixuan-wu edited the summary of this revision. (Show Details)Aug 8 2022, 6:50 PM
zixuan-wu added inline comments.Aug 8 2022, 7:40 PM
clang/test/Driver/riscv-arch.c
410

For now, if the name of sifive-s21 can't be both RV32 and RV64. It must distinguish the name in RISCVTargetParser.def.

414

IMO, if -march is accepted to override riscv32 target triple, and march can get by riscv::getRISCVArch function with -mabi, then it should also accept. Or riscv::getRISCVArch should not get -march from -mabi.

zixuan-wu marked an inline comment as not done.Aug 23 2022, 7:07 PM

ping...

MaskRay added a comment.EditedThu, Sep 15, 8:17 PM

Both D54214 and this look like a surprising behavior to me. Do we still have time to revert D54214 and make mismatching --target & -march= an error?

clang -target=riscv32-- -march=rv64gc

-target= => --target=

For --target=riscv64 -march=rv64i -m32, -march= appears to take precedence, but the rule seems confusing.

Both D54214 and this look like a surprising behavior to me. Do we still have time to go back the state before D54214 and make mismatching --target & -march= an error?

Then there's no -m32 equivalent; that's what -march currently gives you... also GCC lets you do it. And -target is Clang-specific so you can't write something that works for both compilers.

Both D54214 and this look like a surprising behavior to me. Do we still have time to go back the state before D54214 and make mismatching --target & -march= an error?

Then there's no -m32 equivalent; that's what -march currently gives you... also GCC lets you do it. And -target is Clang-specific so you can't write something that works for both compilers.

We can accept -m32/-m64 as a special case. Other -m options

(I edited my previous comment and added a confusing --target=riscv64 -march=rv64i -m32 case when you made the comment.)

AFAIK, --target is clang-specific and transparent for compiler user in RV side. -m is also undefined or less used by RV user. It just uses -march to specify 32 or 64 mode and extensions.
Is the convention specified in RV Spec?