This is an archive of the discontinued LLVM Phabricator instance.

[RISCV] Set triple based on -march flag
ClosedPublic

Authored by simoncook on Nov 7 2018, 10:24 AM.

Details

Summary

For RISC-V the value provided to -march should determine whether to compile for 32- or 64-bit RISC-V irrespective of the target provided to the Clang driver. This adds a test for this flag for RISC-V and sets the Target architecture correctly in these cases.

Diff Detail

Event Timeline

simoncook created this revision.Nov 7 2018, 10:24 AM
simoncook updated this revision to Diff 212772.Aug 1 2019, 3:18 AM

Rebase on top of tree.

I've noticed that without this flag calling riscv32-unknown-elf-clang -march=rv64i will still produce a rv32 binary, which is unlike GCC's behavior

Herald added a project: Restricted Project. · View Herald TranscriptAug 1 2019, 3:18 AM
lenary added a comment.Aug 1 2019, 3:40 AM

What happens if I pass clang -march=rv32i -target riscv64-unknown-elf? Should we care about the ordering of -march vs -target?

What happens if I pass clang -march=rv32i -target riscv64-unknown-elf? Should we care about the ordering of -march vs -target?

Currently this patch makes the output be generated for rv32i. I think that -march should override -target since it is more fine grained, is used as the default way of controlling extensions, and may be less confusing than last XLEN wins.

Ping, before I rebased this did anyone have any other thoughts on flag precedence?

Ping, before I rebased this did anyone have any other thoughts on flag precedence?

The precedence seems sensible; it's what MIPS is doing for -mabi vs -target just above.

clang/lib/Driver/Driver.cpp
545

Missing a full-stop here

lenary added a comment.Nov 4 2019, 6:38 AM

I have no issue with the underlying logic here.

Please rebase on top of D69383 and use riscv::getRISCVArch(...) defined in that patch rather than querying Args.getLastArg(options::OPT_march_EQ) directly.

simoncook updated this revision to Diff 228261.Nov 7 2019, 9:39 AM

Rebase.

@lenary Following the discussion regarding D69383, I think it's best for now to keep the logic just keeping -march directly, rather than using getRISCVArch. I think in the case of -target risc32-..... -mabi=lp64 I think it would confuse users if the tools suddenly changed to doing an rv64 compile. If we disable that, all that function would provide me is the same StringRef I'm already evaluating. I think adding any extra flag to indicate whether a rv32<->rv64 switch is acceptable would just make the code unnecessarily more messy. I think in the future if getRISCVArch evaluates more flags, then it might make sense to reconsider this.

lenary accepted this revision.Nov 7 2019, 9:46 AM

Rebase.

@lenary Following the discussion regarding D69383, I think it's best for now to keep the logic just keeping -march directly, ...

Yep, I understand.

Ok, Logic and tests LGTM. I think this needs an entry in the Release Notes too.

This revision is now accepted and ready to land.Nov 7 2019, 9:46 AM
This revision was automatically updated to reflect the committed changes.

Rebase.

@lenary Following the discussion regarding D69383, I think it's best for now to keep the logic just keeping -march directly, rather than using getRISCVArch. I think in the case of -target risc32-..... -mabi=lp64 I think it would confuse users if the tools suddenly changed to doing an rv64 compile. If we disable that, all that function would provide me is the same StringRef I'm already evaluating. I think adding any extra flag to indicate whether a rv32<->rv64 switch is acceptable would just make the code unnecessarily more messy. I think in the future if getRISCVArch evaluates more flags, then it might make sense to reconsider this.

I think the case of -target risc32-..... -mabi=lp64 is acceptable since the -target is less usable than -march/-mabi/-mcpu and is almost invisible to users.

rkruppe removed a subscriber: rkruppe.Jul 15 2022, 8:41 AM