Page MenuHomePhabricator

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

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

Details

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

Repository
rL LLVM

Event Timeline

apazos created this revision.Dec 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 inline comments.Dec 14 2017, 11:48 PM
test/Driver/riscv-gnutools.c
6 ↗(On Diff #127062)

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
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.

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

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.

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

This revision was automatically updated to reflect the committed changes.

Committed R322769