Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Generate configuration files for clang/clang++ #1548

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

wangpc-pp
Copy link
Collaborator

For GCC, the default -march/-mabi/-mtune can be specified via configuring with --with-arch, --with-abi and --with-tune. But these options won't take affect for Clang/LLVM.

For Clang/LLVM, we can use configuration files[1] to specify default options for different tools/targets. So we automatically generate two configuration files (for clang and clang++) and specify these options in them.

References:
[1] https://clang.llvm.org/docs/UsersManual.html#configuration-files

For GCC, the default `-march`/`-mabi`/`-mtune` can be specified
via configuring with `--with-arch`, `--with-abi` and `--with-tune`.
But these options won't take affect for Clang/LLVM.

For Clang/LLVM, we can use configuration files[1] to specify default
options for different tools/targets. So we automatically generate
two configuration files (for `clang` and `clang++`) and specify
these options in them.

References:
[1] https://clang.llvm.org/docs/UsersManual.html#configuration-files
Copy link
Collaborator

@cmuellner cmuellner left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks!
That's a good idea, but I haven't used the mechanism so far (so not much experience).
Assuming that command line flags have higher priority (i.e., can override the config file), this should not have any negative side-effects.

Makefile.in Outdated Show resolved Hide resolved
Makefile.in Outdated Show resolved Hide resolved
@kito-cheng
Copy link
Collaborator

We tried that before in our downstream, but we found that will break -mcpu and -mabi guessing mechanism.

Let me describe that in more detail way:

  • -mcpu: riscv target will use following rule for clang for pinking up the ISA[1]

    • Use ISA configuration from -march if -march is given
    • Use ISA configuration from -mcpu if -mcpu is given and -march is not given.
    • Use the default ISA based on target triple.
  • Scheduling model will use following rule for clang

    • Use scheduling model from -mtune if -mtune is given
    • Use scheduling model configuration from -mcpu if -mcpu is given and -mtune is not given.
    • Use generic-rv32 or generic-rv64

And also similar case for ABI:

  • Use ABI from -mabi if -mabi is given
  • Otherwise will guess from ISA, based on the rule describe above.

[1] https://github.com/llvm/llvm-project/blob/main/clang/lib/Driver/ToolChains/Arch/RISCV.cpp#L249
[2] https://github.com/llvm/llvm-project/blob/main/clang/lib/Driver/ToolChains/Arch/RISCV.cpp#L188

So why it break that mechanism? because the checking within those rule is based on those option is present or not, however put -march, -mabi, -mtune into configuration file will made clang feel those option are present, and then that mechanism broken :(

Ok, so how we deal that in our downstream? Just change the default string within the source tree :P...
Another possible way is adding few more cmake configuration options to setting the default value.

@kito-cheng
Copy link
Collaborator

BTW I found the complicate rule for -mcpu and -march isn't document anywhere, I thought I have documented in https://github.com/riscv-non-isa/riscv-toolchain-conventions but apparently not.

@wangpc-pp
Copy link
Collaborator Author

wangpc-pp commented Sep 5, 2024

We tried that before in our downstream, but we found that will break -mcpu and -mabi guessing mechanism.

Let me describe that in more detail way:

* `-mcpu`: riscv target will use following rule for clang for pinking up the ISA[1]
  
  * Use ISA configuration from `-march` if `-march` is given
  * Use ISA configuration from `-mcpu` if `-mcpu` is given and  `-march` is not given.
  * Use the default ISA based on target triple.

* Scheduling model will use following rule for clang
  
  * Use scheduling model from `-mtune` if `-mtune` is given
  * Use scheduling model configuration from `-mcpu` if `-mcpu` is given and  `-mtune` is not given.
  * Use generic-rv32 or generic-rv64

And also similar case for ABI:

* Use ABI from `-mabi` if `-mabi` is given

* Otherwise will guess from ISA, based on the rule describe above.

[1] https://github.com/llvm/llvm-project/blob/main/clang/lib/Driver/ToolChains/Arch/RISCV.cpp#L249 [2] https://github.com/llvm/llvm-project/blob/main/clang/lib/Driver/ToolChains/Arch/RISCV.cpp#L188

So why it break that mechanism? because the checking within those rule is based on those option is present or not, however put -march, -mabi, -mtune into configuration file will made clang feel those option are present, and then that mechanism broken :(

Ok, so how we deal that in our downstream? Just change the default string within the source tree :P... Another possible way is adding few more cmake configuration options to setting the default value.

I think I have tried to add a CMake option to specify the default CPU in https://reviews.llvm.org/D144853, and @MaskRay suggested we'd better use config files. :-)

And, is it really a break for these mechanisms? I think the behavior is expected from my personal perspective, we can just pretend that we will always specify these options.

@kito-cheng
Copy link
Collaborator

kito-cheng commented Sep 5, 2024

That's kinda nullify the effect of -mcpu silently if both are --with-tune and --with-arch are given, so yes, I would say it break these mechanisms

@wangpc-pp
Copy link
Collaborator Author

That's kinda nullify the effect of -mcpu silently if both are --with-tune and --with-arch are given, so yes, I would say it break these mechanisms

Oh I get what you mean! Yeah, it is a break. :-(
Maybe we can make configuration files more flexible? Like we can add options optionally:

@add_if_inexistent -mcpu=xxx
@add_if_inexistent -mtune=xxx
@add_if_inexistent -mabi=xxx

@kito-cheng
Copy link
Collaborator

Oh I get what you mean! Yeah, it is a break. :-( Maybe we can make configuration files more flexible? Like we can add options optionally:

@add_if_inexistent -mcpu=xxx
@add_if_inexistent -mtune=xxx
@add_if_inexistent -mabi=xxx

Sounds a good way to go :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants