This is an archive of the discontinued LLVM Phabricator instance.

Differential D41271

[RISCV] Propagate -mabi and -march values to GNU assembler.
ClosedPublic

Authored by apazos on Dec 14 2017, 8:30 PM.

Details

Reviewers
apazos
asb
Commits
rGf4b1c002d1ec: [RISCV] Propagate -mabi and -march values to GNU assembler.
rC322769: [RISCV] Propagate -mabi and -march values to GNU assembler.
rL322769: [RISCV] Propagate -mabi and -march values to GNU assembler.
Summary

When using -fno-integrated-as flag, the gnu assembler produces code with some default march/mabi which later causes linker failure due to incompatible mabi/march.

In this patch we explicitly propagate -mabi and -march flags to the GNU assembler.

Observations:

  • Should we also propagate these values to the linker besides the assembler? I think we should. I can update the patch if you agree.
  • There is no default value for march. Should we set a default one?

Diff Detail

Event Timeline

apazos created this revision.Dec 14 2017, 8:30 PM
Herald added subscribers: sabuasal, jordy.potman.lists, simoncook and 2 others. · View Herald TranscriptDec 14 2017, 8:30 PM
asb edited subscribers, added: cfe-commits; removed: llvm-commits.Dec 14 2017, 11:44 PM

Removing llvm-commits and adding cfe-commits.

Thanks Ana. This looks good to me. We probably should have a default march, but I don't know what it should be. If most clang targets default to a Linux-capable baseline it should probably be -march=rv32gc or -march=rv64gc depending on if the target is rv32/rv64.

asb added a parent revision: D39963: [RISCV] Add initial RISC-V target and driver support.Dec 14 2017, 11:45 PM
asb added inline comments.Dec 14 2017, 11:48 PM
test/Driver/riscv-gnutools.c
7

s/MABI-ILP64/MABI-LP64

apazos updated this revision to Diff 127170.Dec 15 2017, 11:46 AM

changed label prefix ILP64 to LP64

apazos added a comment.Jan 8 2018, 1:02 PM

This is ready to merge, just waiting for the dependence D39963 to be merged first.

apazos accepted this revision.Jan 8 2018, 1:02 PM
This revision is now accepted and ready to land.Jan 8 2018, 1:02 PM
asb accepted this revision.Jan 8 2018, 1:08 PM
apazos updated this revision to Diff 129476.Jan 11 2018, 10:36 AM

Rebased.

Herald added a subscriber: niosHD. · View Herald TranscriptJan 11 2018, 10:36 AM
asb added a comment.Jan 11 2018, 10:47 AM

I've just had a painful time landing D39963 due to failures on the Windows buildbots. I think you'll have problems on Windows with the tests in this patch, as Windows surely won't default to /usr/bin/as. Sadly I don't have access to a windows Clang build to confirm what path it will default to.

apazos added a comment.Jan 11 2018, 6:40 PM

thanks Alex, I will test on windows before pushing.

apazos updated this revision to Diff 129981.Jan 16 2018, 9:55 AM

I tested this on windows and I had to add an assembler placeholder executable, just like it was done with the linker in the RISCV multilib dir checked under Inputs.
Other observations, are these known issues?

  • multilib dir checked in has only riscv64-unknown-linux-gnu which we see the riscv tests invoking even when the target is 32 bit.
  • riscv64 is not honoring gcc-toolchain flag, it keeps invoking default /usr/bin/as.

Anyways, the test now passes on windows.

asb added a comment.Jan 17 2018, 6:01 AM
In D41271#977222, @apazos wrote:

I tested this on windows and I had to add an assembler placeholder executable, just like it was done with the linker in the RISCV multilib dir checked under Inputs.
Other observations, are these known issues?

  • multilib dir checked in has only riscv64-unknown-linux-gnu which we see the riscv tests invoking even when the target is 32 bit.
  • riscv64 is not honoring gcc-toolchain flag, it keeps invoking default /usr/bin/as.

Anyways, the test now passes on windows.

For the first issue: I've matched the multilib toolchain generated from the riscv-gnu-toolchain build scripts, which uses the riscv64-* triple prefix throughout (see https://github.com/riscv/riscv-gnu-toolchain/issues/296).
For the second: driver support for riscv64 hasn't been added, as we don't yet have codegen support upstream. Codegen support doesn't really block driver+tests of course, so we can add riscv64 if it's useful to you.

apazos added a comment.Jan 17 2018, 1:37 PM

Thanks for the clarifications Alex.
No need to fix these known issues right now.
I will proceed with checking in this change.

Closed by commit rL322769: [RISCV] Propagate -mabi and -march values to GNU assembler. (authored by apazos). · Explain WhyJan 17 2018, 2:11 PM
This revision was automatically updated to reflect the committed changes.
apazos added a comment.Jan 17 2018, 2:12 PM

Committed R322769

Revision Contents

PathSize
lib/
Driver/
ToolChains/
12 lines
test/
Driver/
Inputs/
multilib_riscv_linux_sdk/
riscv64-unknown-linux-gnu/
bin/
1 line
14 lines

Diff 129981

lib/Driver/ToolChains/Gnu.cpp

Show First 20 Lines • Show All 623 Lines • ▼ Show 20 Linesvoid tools::gnutools::Assembler::ConstructJob(Compilation &C,
case llvm::Triple::ppc64le: { case llvm::Triple::ppc64le: {
CmdArgs.push_back("-a64"); CmdArgs.push_back("-a64");
CmdArgs.push_back("-mppc64"); CmdArgs.push_back("-mppc64");
CmdArgs.push_back("-mlittle-endian"); CmdArgs.push_back("-mlittle-endian");
CmdArgs.push_back( CmdArgs.push_back(
ppc::getPPCAsmModeForCPU(getCPUName(Args, getToolChain().getTriple()))); ppc::getPPCAsmModeForCPU(getCPUName(Args, getToolChain().getTriple())));
break; break;
} }
case llvm::Triple::riscv32:
case llvm::Triple::riscv64: {
StringRef ABIName = riscv::getRISCVABI(Args, getToolChain().getTriple());
CmdArgs.push_back("-mabi");
CmdArgs.push_back(ABIName.data());
if (const Arg *A = Args.getLastArg(options::OPT_march_EQ)) {
StringRef MArch = A->getValue();
CmdArgs.push_back("-march");
CmdArgs.push_back(MArch.data());
}
break;
}
case llvm::Triple::sparc: case llvm::Triple::sparc:
case llvm::Triple::sparcel: { case llvm::Triple::sparcel: {
CmdArgs.push_back("-32"); CmdArgs.push_back("-32");
std::string CPU = getCPUName(Args, getToolChain().getTriple()); std::string CPU = getCPUName(Args, getToolChain().getTriple());
CmdArgs.push_back(sparc::getSparcAsmModeForCPU(CPU, getToolChain().getTriple())); CmdArgs.push_back(sparc::getSparcAsmModeForCPU(CPU, getToolChain().getTriple()));
AddAssemblerKPIC(getToolChain(), Args, CmdArgs); AddAssemblerKPIC(getToolChain(), Args, CmdArgs);
break; break;
} }
▲ Show 20 Lines • Show All 1,808 Lines • Show Last 20 Lines

test/Driver/Inputs/multilib_riscv_linux_sdk/riscv64-unknown-linux-gnu/bin/as

  • This file was added.
PropertyOld ValueNew Value
File Modenull100755
#!/bin/true

test/Driver/riscv-gnutools.c

  • This file was added.
// Check gnutools are invoked with propagated values for -mabi and -march.
// RUN: %clang -target riscv32-linux-unknown-elf -fno-integrated-as \
// RUN: --gcc-toolchain=%S/Inputs/multilib_riscv_linux_sdk \
// RUN: --sysroot=%S/Inputs/multilib_riscv_linux_sdk/sysroot %s -### \
// RUN: 2>&1 | FileCheck -check-prefix=MABI-ILP32 %s
// RUN: %clang -target riscv32-linux-unknown-elf -fno-integrated-as \
asbUnsubmitted
Not Done
ReplyInline Actions

s/MABI-ILP64/MABI-LP64

asb: s/MABI-ILP64/MABI-LP64
// RUN: -march=rv32g --gcc-toolchain=%S/Inputs/multilib_riscv_linux_sdk \
// RUN: --sysroot=%S/Inputs/multilib_riscv_linux_sdk/sysroot %s -### \
// RUN: 2>&1 | FileCheck -check-prefix=MABI-ILP32-MARCH-G %s
// MABI-ILP32: "{{.*}}/Inputs/multilib_riscv_linux_sdk/lib/gcc/riscv64-unknown-linux-gnu/7.2.0/../../../../riscv64-unknown-linux-gnu/bin{{/|\\\\}}as" "-mabi" "ilp32"
// MABI-ILP32-MARCH-G: "{{.*}}/Inputs/multilib_riscv_linux_sdk/lib/gcc/riscv64-unknown-linux-gnu/7.2.0/../../../../riscv64-unknown-linux-gnu/bin{{/|\\\\}}as" "-mabi" "ilp32" "-march" "rv32g"