Page MenuHomePhabricator

[RISCV] Add -mtune support
ClosedPublic

Authored by kito-cheng on Oct 7 2020, 8:45 PM.

Details

Summary
  • The goal of this patch is improve option compatible with RISCV-V GCC, -mcpu support on GCC side will sent patch in next few days.
  • -mtune only affect the pipeline model and non-arch/extension related target feature, e.g. instruction fusion; in td file it called TuneFeatures, which is introduced by X86 back-end[1].
  • -mtune accept all valid option for -mcpu and extra alias processor option, e.g. generic, rocket and sifive-7-series, the purpose is option compatible with RISCV-V GCC.
  • Processor alias for -mtune will resolve according the current target arch, rv32 or rv64, e.g. rocket will resolve to rocket-rv32 or rocket-rv64.
  • Interaction between -mcpu and -mtune:
    • -mtune has higher priority than -mcpu for pipeline model and TuneFeatures.

[1] https://reviews.llvm.org/D85165

Diff Detail

Event Timeline

kito-cheng created this revision.Oct 7 2020, 8:45 PM
kito-cheng requested review of this revision.Oct 7 2020, 8:45 PM

RISCV supports -mcpu with default empty arch to align gcc's -mtune behavior since clang didn't support -mtune before. But now clang has -mtune, is it a good idea to remove those options? (ex. rocket-rv32/rv64, sifive-7-rv32/64)

clang/test/Driver/riscv-cpus.c
82

maybe we can describe what is expected interaction behavior somewhere.

luismarques accepted this revision.Oct 13 2020, 8:11 AM

LGTM, but I would like other people to also review this, if possible.
(Just be sure to check/fix the clang-format warnings and the inline comments).

clang/test/Driver/riscv-cpus.c
29

Nit: resolve -> resolved.

82

+1

llvm/lib/Target/RISCV/RISCVAsmPrinter.cpp
201–203

Nit: don't -> doesn't; for -> of; attribute -> attributes.

This revision is now accepted and ready to land.Oct 13 2020, 8:11 AM

RISCV supports -mcpu with default empty arch to align gcc's -mtune behavior since clang didn't support -mtune before. But now clang has -mtune, is it a good idea to remove those options? (ex. rocket-rv32/rv64, sifive-7-rv32/64)

If possible that would good, since -mcpu is deprecated (for e.g. x86_64) or unsupported in GCC (for e.g. RISC-V). So doing that would further align Clang with GCC. But I wonder if this might be too problematic, in terms of compatibility.

If possible that would good, since -mcpu is deprecated (for e.g. x86_64) or unsupported in GCC (for e.g. RISC-V). So doing that would further align Clang with GCC. But I wonder if this might be too problematic, in terms of compatibility.

I am also working on -mcpu support for RISC-V GCC, and I expect it would included in next GCC release, so -mcpu and -mtune would align between both compilers.
https://gcc.gnu.org/pipermail/gcc-patches/2020-October/556058.html

ChangeLog

  • Fix wording in comment
  • Add more comment in testcase
  • Fix format issue.
kito-cheng marked 3 inline comments as done.Oct 14 2020, 1:06 AM

RISCV supports -mcpu with default empty arch to align gcc's -mtune behavior since clang didn't support -mtune before. But now clang has -mtune, is it a good idea to remove those options? (ex. rocket-rv32/rv64, sifive-7-rv32/64)

If possible that would good, since -mcpu is deprecated (for e.g. x86_64) or unsupported in GCC (for e.g. RISC-V). So doing that would further align Clang with GCC. But I wonder if this might be too problematic, in terms of compatibility.

Personally I would like to remove rocket-rv32/rv64, sifive-7-rv32/64, but I didn't remove rocket-rv32/rv64, sifive-7-rv32/64 in version 2 patch, since I concern about compatibility too, Clang/LLVM 11 already included that, I would prefer create another patch to remove that and discuss that issue.

luismarques accepted this revision.Oct 14 2020, 1:58 AM
MaskRay added inline comments.Oct 14 2020, 9:48 AM
clang/test/Driver/riscv-cpus.c
91

A NOT pattern depends on the feature order and thus a bit unreliable.
Please use -SAME whenever appropriate

ChangeLog:

  • Update testcase according to MaskRay's suggestion.
kito-cheng marked an inline comment as done.Oct 14 2020, 8:43 PM

@MaskRay Thanks, that's first time I know the suffix -SAME :P

This revision was automatically updated to reflect the committed changes.