Page MenuHomePhabricator

[RISCV] support clang driver to select cpu
ClosedPublic

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

Details

Summary

update implementation and handle -mcpu with explicitly specified -march.
If -mcpu has default march, explicitly -march will overwrite it.

  1. reference ARM/AArch64 and X86 to implement RISCVTargetParser.def
  2. in clang, -mtune option alias to -mcpu. This patch supports -mtune option via -mcpu.
  3. propose two category of CPUs in TargetParser and backend. First is similar gcc's -mtune option (it includes micro architecture related feature only, ex. rocket-rv32 has different schedule model), another also has default -march (isa string).
  4. add two sifive cpu which has default -march
  5. changed default march logic.

march: in order:

  1. Explicit choices using -march
  2. A default based on -mcpu, if target cpu has default -march
  3. default based on -mabi, if provided
  4. A default based on the target triple's arch

I think it's also okay if we want to only support -mcpu which is similar to gcc's -mtune at the moment . Because in current implementation,
users can query available CPUs via -mcpu=? on clang but mcpu option is not workable.

Diff Detail

Event Timeline

There are a very large number of changes, so older changes are hidden. Show Older Changes
Jim added inline comments.Dec 9 2019, 12:50 AM
clang/lib/Basic/Targets/RISCV.h
47

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
171

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
171

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

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

yes, you are right, thanks.

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

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
171

@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
171

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 .

  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,

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

khchen updated this revision to Diff 255377.EditedApr 6 2020, 10:16 AM
khchen added reviewers: evandro, HsiangKai.

update implementation and handle -mcpu with explicitly specified -march.
If -mcpu has default march, explicitly -march will overwrite it.

  1. reference ARM/AArch64 and X86 to implement RISCVTargetParser.def
  2. in clang, -mtune option alias to -mcpu. This patch supports -mtune option via -mcpu.
  3. propose two category of CPUs in TargetParser and backend. First is similar gcc's -mtune option (it includes micro architecture related feature only, ex. rocket-rv32 has different schedule model), another also has default -march (isa string).
  4. add two sifive cpu which has default -march
  5. changed default march logic.

march: in order:

  1. Explicit choices using -march
  2. A default based on -mcpu, if target cpu has default -march
  3. A default based on -mabi, if provided
  4. A default based on the target triple's arch

I think it's also okay if we want to only support -mcpu which is similar to gcc's -mtune at the moment . Because in current implementation,
users can query available CPUs via -mcpu=? on clang but mcpu option is not workable.

@lenary (Sorry for the very late reply...)

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

Yes, It's my mistake, I mean -mtune.

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 .

  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,

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

Yes, so lowRISC's Ibex need to use different pipleine model.
I mention it just because in SiFive, the same serial core can share the same schedule model, but they have different isa extension.

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

thanks for reminding me, I re-implementation it.

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 .

  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,

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

khchen edited the summary of this revision. (Show Details)Apr 6 2020, 10:26 AM

This is looking good.

I remember we discussed this on the LLVM call a few weeks ago - there was a discussion as to whether we should be prioritising -march or -mcpu - do you recall the outcome of that discussion?

clang/lib/Basic/Targets/RISCV.cpp
172

It would be good to have /*Is64Bit=*/ before the boolean arguments to these functions.

clang/lib/Driver/ToolChains/Arch/RISCV.cpp
593–608

What's the justification for removing this code?

clang/lib/Driver/ToolChains/CommonArgs.cpp
338

Should we be doing more validation here?

llvm/lib/Support/TargetParser.cpp
122

I think this should be prefixed RISCV, or moved into the RISCV namespace if you can?

246–247

It might be worth explicitly adding -64bit where FK_64BIT is not set.

Herald added a project: Restricted Project. · View Herald TranscriptApr 29 2020, 12:04 PM

Another proposal for -mcpu and -mtune:

Decoupling the -mcpu and -mtune option, -mcpu only accept concrete CPU, and -mtune for micro-arch/pipeline model, they accept different option set.

e.g.
-mcpu=sifive-e24 # Imply -march=rv32imafc -mtune=sifive-2-series
-mtune=sifive-2-series # no effect on arch
-mtune=rocket # no effect on arch

So -mcpu=rocket is invalid, since it's micro-arch/pipeline model only,
and -mtune=sifive-e24 is invalid too, because it's CPU not a micro-arch.

khchen updated this revision to Diff 261165.Apr 30 2020, 2:45 AM
khchen marked 9 inline comments as done.

address @lenary's review feedback

This is looking good.

I remember we discussed this on the LLVM call a few weeks ago - there was a discussion as to whether we should be prioritising -march or -mcpu - do you recall the outcome of that discussion?

Sorry, I don't remember the outcome of discussion...
I thought it make sense if both mcpu and march are specified, and march extensions overrides the mcpu extension
This also allows users to specific superset or subset of march.

clang/lib/Driver/ToolChains/Arch/RISCV.cpp
593–608

I changed the logic in how to get the default march by calling getRISCVArch(Args, Triple);
getRISCVArch can also get the default march based on the triple, so we don't need above code now.

clang/lib/Driver/ToolChains/CommonArgs.cpp
338

I think we don't need to validation here, we have validation in RISCVTargetInfo::isValidCPUName. You can see it work in test/Driver/riscv-cpus.c#L22.

Another proposal for -mcpu and -mtune:

Decoupling the -mcpu and -mtune option, -mcpu only accept concrete CPU, and -mtune for micro-arch/pipeline model, they accept different option set.

e.g.
-mcpu=sifive-e24 # Imply -march=rv32imafc -mtune=sifive-2-series
-mtune=sifive-2-series # no effect on arch
-mtune=rocket # no effect on arch

So -mcpu=rocket is invalid, since it's micro-arch/pipeline model only,
and -mtune=sifive-e24 is invalid too, because it's CPU not a micro-arch.

I agree this is better proposal but clang treats -mtune and -mcpu as same options now...
ex. https://github.com/llvm-mirror/clang/blob/master/include/clang/Driver/Options.td#L2751-L2752

kito-cheng added inline comments.Apr 30 2020, 7:56 AM
clang/lib/Driver/ToolChains/Arch/RISCV.cpp
569

I think it worth to keep this comment untouched, since it describing GCC's behavior.

647

This part seems like is describing about GCC's behavior, so it should not change unless GCC changed?

khchen updated this revision to Diff 261741.May 3 2020, 11:48 PM

address kito's comment

khchen marked 3 inline comments as done.May 3 2020, 11:48 PM
swpenim added a subscriber: swpenim.Jul 6 2020, 7:25 PM
lenary added a comment.Jul 8 2020, 3:31 PM

I realise this is almost certainly something we want to land before the LLVM 11 branch date, as we included schedules in LLVM 10 with no way to use them, and would like users to be able to use them. I'll bring it up on the call tomorrow - I hope this PR implements what we agreed from the previous calls.

asb added a comment.EditedJul 8 2020, 9:52 PM

This has been hanging around for a while, but I think we'd basically agreed this is the right logic. The comments have ended up referring to flags that don't exist on Clang making it a little hard to follow, and I've added a request to slightly expand testing. If you make those cleanups I think it should be ready for a final review and merge.

As Sam says, lets flag this in today's RISC-V LLVM call to double-check everyone is happy.

EDIT: this patch also needs a rebase

clang/lib/Driver/ToolChains/Arch/RISCV.cpp
654

As clang has no with-arch or with-abi, this comment seems inaccurate?

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

I think for completeness this test should be validating the interaction of the ABI choosing logic with CPU selection as well. With the implemented logic I believe it should show that lp64d is selected for -mcpu=sifive-u54 and that -mcpu=sifive-u54 -mabi=lp64 will respect the ABI choice

khchen planned changes to this revision.Jul 9 2020, 6:40 AM
khchen marked an inline comment as done.

BTW, this patch depends on D77030, which aim to avoid the forcing of any ProcessorModel to have FeatureRVCHints feature.
But if we decide to keep the FeatureRVCHints, I need to change implementation a little.

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

okay, it makes sense to me, I will update this patch soon.

khchen updated this revision to Diff 276951.EditedJul 10 2020, 1:48 AM

addess @asb 's comment.

[RISCV][Clang] Support -mcpu option.

Summary:

  1. gcc uses -march and -mtune flag to chose arch and

pipeline model, but clang does not have -mtune flag,
we uses -mcpu to chose both info.

  1. Add SiFive e31 and u54 cpu which have default march

and pipeline model.

  1. Specific -mcpu with rocket-rv[32|64] would select

pipeline model only, and use the driver's arch choosing
logic to get default arch.

khchen marked 2 inline comments as done.Jul 10 2020, 1:50 AM
khchen updated this revision to Diff 277042.Jul 10 2020, 8:00 AM

fix typo

khchen updated this revision to Diff 277067.Jul 10 2020, 9:06 AM

avoid to check compiler version in testcase

@asb @lenary I thought this path is ready to land?

I've got one major issue (inline below), and I'm confused by some other behaviour:

When I run clang --target=riscv64 -mcpu=?, the list includes both generic-rv32 and generic-rv64. It doesn't show only the 64-bit cpus. This is not changed by giving a full triple, or by additionally specifying -march=rv64gc. Should it be?

For instance, this output:

$ clang --target=riscv64-unknown-elf -mcpu=\? -march=rv64gc
clang version 11.0.0
Target: riscv64-unknown-unknown-elf
Thread model: posix
InstalledDir: /home/selliott/llvm-project/./build/llvm_clang/all/bin
Available CPUs for this target:

	generic-rv32
	generic-rv64
	rocket-rv32
	rocket-rv64
	sifive-e31
	sifive-u54

Use -mcpu or -mtune to specify the target's processor.
For example, clang --target=aarch64-unknown-linux-gui -mcpu=cortex-a35
clang/lib/Driver/ToolChains/Arch/RISCV.cpp
626

I now get a stacktrace here if I call clang --target=riscv32 -march=unknown foo.c -c. In clang-10, you get a nice error:

clang-10: error: invalid arch name 'wat', string must begin with rv32{i,e,g} or rv64{i,g}

The stacktrace is coming from the call to getRISCVABI in findRISCVBareMetalMultilibs (in clang/lib/Driver/ToolChains/Gnu.cpp:1608).

Hi @lenary
This is normal behavior in current clang implementation,
-mcpu=? flag does not interact with any flags, it just print out the all of ProcessorModel registered in backend.
https://github.com/llvm/llvm-project/blob/master/clang/tools/driver/cc1_main.cpp#L213-L215

You can reference this, so the clang -mcpu=? result is exactly equal to ./bin/llc -mattr=+cpuhelp.

./bin/llc -mattr=+cpuhelp  foo.ll  --mtriple=riscv32
Available CPUs for this target:

        generic-rv32
        generic-rv64
        rocket-rv32
        rocket-rv64

Use -mcpu or -mtune to specify the target's processor.
For example, clang --target=aarch64-unknown-linux-gui -mcpu=cortex-a35

You can try clang -mcpu=? --target=mips64el or other target too.

But you are right, this behavior is very confusing users.

I've got one major issue (inline below), and I'm confused by some other behaviour:

When I run clang --target=riscv64 -mcpu=?, the list includes both generic-rv32 and generic-rv64. It doesn't show only the 64-bit cpus. This is not changed by giving a full triple, or by additionally specifying -march=rv64gc. Should it be?

For instance, this output:

$ clang --target=riscv64-unknown-elf -mcpu=\? -march=rv64gc
clang version 11.0.0
Target: riscv64-unknown-unknown-elf
Thread model: posix
InstalledDir: /home/selliott/llvm-project/./build/llvm_clang/all/bin
Available CPUs for this target:

	generic-rv32
	generic-rv64
	rocket-rv32
	rocket-rv64
	sifive-e31
	sifive-u54

Use -mcpu or -mtune to specify the target's processor.
For example, clang --target=aarch64-unknown-linux-gui -mcpu=cortex-a35
asb added a comment.Jul 14 2020, 10:14 PM

I've added some suggestions to clarify the code comments. I think before landing it would be good to address the crash Sam pointed out for an invalid -march, but otherwise I think this looks good to me (at least, it seems worth landing this and if further issues crop up we can fix them and request them for merging into the LLVM 11 branch).

clang/lib/Driver/ToolChains/Arch/RISCV.cpp
566

"The logic used in GCC 9.2.0 i the following, in order:"

576

"Clang uses the following logic;"

579

"A default based on the architecture as determined by getRISCVArch"

586

"Choose a default based on the target architecture"

642

"The logic used in GCC 9.2.0 is the following, in order:"

654

Based on -mcpu if the target CPU has a default ISA string

clang/lib/Driver/ToolChains/Arch/RISCV.h
29 ↗(On Diff #277067)

These functions aren't defined anywhere - delete?

khchen updated this revision to Diff 278106.Jul 15 2020, 1:42 AM
khchen marked 8 inline comments as done.

address @asb's comment. thanks.

Thanks for @lenary bug report.
I cannot reproduce the crash in local,
but I though the problem is cause by removing "Choose a default based on the triple" logic in getRISCVABI
The MArch value is "unknown" and there is no return value which can match any conditionin.
so I add it back and also update the "invalid arch name" test.

Thanks for the fix!

Please can you also update clang/test/Misc/target-invalid-cpu-note.c for riscv32 and riscv64 - it's likely you'll need the fix in the inline note below.

Also, there are a bunch of clang-tidy issues, which I think we can fix now rather than later.

llvm/lib/Support/TargetParser.cpp
250

This line should be if (C.Kind != CK_INVALID && IsRV64 == C.Is64Bit()) so that when clang prints out valid CPUs, 'invalid' is not included in the list.

khchen updated this revision to Diff 278197.EditedJul 15 2020, 8:17 AM

address @lenary's comment, good catch! thanks a lot!!

khchen marked an inline comment as done.Jul 15 2020, 8:18 AM
lenary accepted this revision.EditedJul 15 2020, 8:22 AM

I am happy with this. I think we should get it landed so we can backport it to the LLVM 11 branch.

I don't know if any of @asb's comments are unresolved. You should probably wait for his OK too.

This revision is now accepted and ready to land.Jul 15 2020, 8:22 AM
asb accepted this revision.Jul 16 2020, 7:06 AM

LGTM, thanks!

This revision was automatically updated to reflect the committed changes.