Page MenuHomePhabricator

[RISCV] support clang driver to select cpu
Changes PlannedPublic

Authored by khchen on Dec 6 2019, 8:42 AM.

Details

Reviewers
lenary
asb
Summary

This is follow up patch for https://reviews.llvm.org/D68685

Diff Detail

Event Timeline

khchen created this revision.Dec 6 2019, 8:42 AM
Jim added inline comments.Dec 9 2019, 12:50 AM
clang/lib/Basic/Targets/RISCV.h
46

I think this should test cpu name is valid first. And assign Name to CPU, if it is valid.

lenary added inline comments.
clang/lib/Basic/Targets/RISCV.cpp
164

Is there not a tablegen'd implementation of these based on https://github.com/llvm/llvm-project/blob/master/llvm/lib/Target/RISCV/RISCV.td#L96-L99 (which will include rocket-rv32 and rocket-rv64 when those two schedules are landed)?

khchen updated this revision to Diff 233082.Dec 10 2019, 6:49 AM
khchen marked 3 inline comments as done.Dec 10 2019, 6:56 AM
khchen added inline comments.
clang/lib/Basic/Targets/RISCV.cpp
164

you are right, if generic-cpu uses rocket chip scheduler, it's okay to abandon this patch.

clang/lib/Basic/Targets/RISCV.h
46

yes, you are right, thanks.

lenary added inline comments.Dec 10 2019, 7:41 AM
clang/lib/Basic/Targets/RISCV.cpp
164

No, that's not quite what I mean. When we add the rocket schedules, there will be additional ProcessorModel entries for the rocket chips, in addition to the current generic models.

The point in these ProcessorModel entries is if a user passes -mcpu=generic-rv64, then [Feature64Bit, FeatureRVCHints] will get turned on, because they chose a specific cpu. This is different to validating that the correct features are enabled in order to choose a cpu, which seems the correct way around.

Then instead of checking against hard-coded lists, you use use MCSubtargetInfo::isCPUStringValid(StringRef), which uses the info from the ProcessorModel tablegen entries.

khchen planned changes to this revision.Dec 19 2019, 10:30 PM
khchen marked an inline comment as done.

The problem is how -mcpu interact with explicitly specified -march (or target features).

  1. in GCC, -mcpu is only used to chose the pipeline model,
  1. I also read this article talking about the X86 and ARM to handle those options.

-march=X: Tells the compiler that X is the minimal architecture the binary must run on. The compiler is free to use architecture-specific instructions. This flag behaves differently on Arm and x86. On Arm, -march does not override -mtune, but on x86 -march will override both -mtune and -mcpu.
-mtune=X: Tells the compiler to optimize for microarchitecture X but does not allow the compiler to change the ABI or make assumptions about available instructions. This flag has the more-or-less the same meaning on Arm and x86.
-mcpu=X: On Arm, this flag is a combination of -march and -mtune. It simultaneously specifies the target architecture and optimizes for a given microarchitecture. On x86 this flag is a deprecated synonym for -mtune.

So maybe it makes sense to treat those flags behaves differently on different target .

  1. I also tried llc to specific -mcpu and -attr (similar to -march, target-feature) in ARM, -attr will over write the default target-feature in -mcpu.

on RISC-V, in sometime (or most?) we have same pipeline model but support different extension combination,
so I think maybe distinguishing the purpose of -mcpu and -march and make them with no interaction is a good idea. (behavior is equal to GCC)

evandro added a subscriber: evandro.Jan 9 2020, 8:37 AM
khchen marked an inline comment as done.Jan 9 2020, 10:07 PM
khchen added inline comments.
clang/lib/Basic/Targets/RISCV.cpp
164

@lenary
I see no backend uses the info (ex. RISCVSubTypeKV?) from tablegen entries,
I saw different targets use different way to impl. TargetInfo::isValidCPUName.
for example:
x86 uses clang/Basic/X86Target.def to record (hard-codeed) valid cpu enum and string,
arm/aarch64 uses llvm/Support/ARMTargetParser.def,
and mips just hard code the valid cpu string in clang/lib/Basic/Targets/Mips.cpp.
They don't use backend to check valid cpu string, so maybe this patch is workable.
But I wonder if you are asking this patch should implement the checking for invalid combination like -mcpu=generic-rv32 with -mattr=+64bit ?

evandro added inline comments.Jan 10 2020, 11:18 AM
clang/lib/Basic/Targets/RISCV.cpp
164

Strange formatting...

The problem is how -mcpu interact with explicitly specified -march (or target features).

  1. in GCC, -mcpu is only used to chose the pipeline model,

I think you mean "in GCC, -mtune is only used to choose the pipeline model" (-mcpu is not documented in the RISC-V specific GCC options documentation).

Clang should attempt to maintain compatibility with GCC flags, but if they only implement -mtune, then we have a little more freedom to do something ergonomic with -mcpu.

I'll note that clang already has a large TODO around implementing -mtune in general, though the AArch64 backend seems to support it for some option choices.

  1. I also read this article talking about the X86 and ARM to handle those options.
    • -march=X: Tells the compiler that X is the minimal architecture the binary must run on. The compiler is free to use architecture-specific instructions. This flag behaves differently on Arm and x86. On Arm, -march does not override -mtune, but on x86 -march will override both -mtune and -mcpu.
    • -mtune=X: Tells the compiler to optimize for microarchitecture X but does not allow the compiler to change the ABI or make assumptions about available instructions. This flag has the more-or-less the same meaning on Arm and x86.
    • -mcpu=X: On Arm, this flag is a combination of -march and -mtune. It simultaneously specifies the target architecture and optimizes for a given microarchitecture. On x86 this flag is a deprecated synonym for -mtune.

      So maybe it makes sense to treat those flags behaves differently on different target .
  2. I also tried llc to specific -mcpu and -attr (similar to -march, target-feature) in ARM, -attr will over write the default target-feature in -mcpu.

    on RISC-V, in sometime (or most?) we have same pipeline model but support different extension combination,

I don't believe this to be correct. lowRISC's Ibex has a completely different pipeline model to rocket, and there are countless other RISC-V cores with different pipeline characteristics, including out-of-order pipeline implementations like BOOM. I don't think we can favour one particular scheduling model (beyond the generic ones we already default to).

so I think maybe distinguishing the purpose of -mcpu and -march and make them with no interaction is a good idea. (behavior is equal to GCC)

In LLVM, if you add target-cpu metadata to a function (which is added by clang, based on -mcpu), that function will have all the features of that CPU automatically added to it (as if you had used -mattr with all the features in the model). If you don't add that metadata, a generic scheduling model will be chosen. This suggests at the moment there can be no separation between -mtune and -march as there is in GCC (without changes to the target-independent parts of LLVM).