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

Initial gcc toolchain configuration for the lowRISC compiler. #1

Merged
merged 3 commits into from
Aug 3, 2021

Conversation

cfrantz
Copy link
Collaborator

@cfrantz cfrantz commented Jul 26, 2021

Configure lowrisc_toolchain_rv32imc for use by the OpenTitan project.

@cfrantz
Copy link
Collaborator Author

cfrantz commented Jul 26, 2021

Things to discuss:

  1. What is the proper name in constraints/cpu/BUILD: riscv32 or rv32imc? Based on the bazelbuild/platforms repo, I feel like riscv32 is the name of the cpu and rv32imc is a more specific architecture-and-features descriptor.

  2. Need to do better on compiler flag selections in toolchains/features/common/impl/gcc.bzl and toolchains/features/embedded/impl/gcc.bzl. I think these flag selections should be data driven and the flags themselves should be moved into the platform or device configurations: platforms/BUILD and/or devices/riscv32/riscv32.bzl. This is a bit of a bigger structural change and will probably require some consultation with the upstream maintainers.

)
if architecture == 'rv32imc':
# TODO(cfrantz): why do I have to do this? Seems the lowRISC gcc
# doesn't support `-mfpu`, `-mfloat-abi` or `-mlittle-endian`.
Copy link
Collaborator

Choose a reason for hiding this comment

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

-mlittle-endian should work (at least it should if we update gcc). AFAICT the other flags are arm specific and aren't defined for other targets. The flag definitions should probably be moved into arch-specific code.

constraint_value(
# Based on the current riscv64 consrtaint in platforms, I wonder
# if this should be `riscv32` or `rv32imc`.
name = "rv32imc",
Copy link
Collaborator

Choose a reason for hiding this comment

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

I'm leaning towards keeping rv32imc. It looks like the generic arm target is slated for removal and there are a bunch of more specific 32-bit arm targets. riscv64 is probably geared towards the non-embedded use case.

Copy link
Collaborator Author

@cfrantz cfrantz Jul 28, 2021

Choose a reason for hiding this comment

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

Based on bazelbuild/platforms#30, riscv32 is the appropriate high-level cpu descriptor. In fact, the change has already been applied by Google's copybara automation to Google internal copy of the platforms repo.

@cfrantz
Copy link
Collaborator Author

cfrantz commented Jul 29, 2021

To be consistent with bazelbuild/platforms#30, I've renamed the cpu to riscv32.

@cfrantz
Copy link
Collaborator Author

cfrantz commented Jul 29, 2021

I think we should commit this change as-is to move the bazel work foward.

I have opened https://github.com/silvergasp/bazel-embedded/issues/37 with upstream to try to get feedback on the proper approach to integrating compiler/feature flags into a "user-serviceable" part of the bazel-embedded repo. Until we have that understanding, its fine to live with the if architecture == 'riscv32' hacks in toolchains/features/{common, embedded}/impl/gcc.bzl.

@mundaym
Copy link
Collaborator

mundaym commented Jul 29, 2021

I think we should commit this change as-is to move the bazel work foward.

SGTM. Feel free to commit this.

Copy link

@mcy mcy left a comment

Choose a reason for hiding this comment

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

Just a couple of format nits. I'm going to assume Michael's LGTM is good enough. =)

"defs.bzl",
"""
SYSTEM_INCLUDE_COMMAND_LINE = {}
SYSTEM_INCLUDE_PATHS= {}
Copy link

Choose a reason for hiding this comment

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

Space after the = sign? =P

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Fixed. These two formatting errors also exist in the file I copied this from (gcc_arm_none_repository.bzl)

"""
filegroup(
name = "all",
srcs = glob(["**/*"],exclude=["**/*.html","**/*.pdf"]),
Copy link

Choose a reason for hiding this comment

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

Missing space after the , too

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Fixed.

Configure lowrisc_toolchain_rv32imc for use by the OpenTitan project.

Signed-off-by: Chris Frantz <cfrantz@google.com>
1. Deliver per-device flags to the GccEmbeddedFeatures function.
2. Turn off some per-feature compiler/linker flags.  Need to find a
   better way to do this.

Signed-off-by: Chris Frantz <cfrantz@google.com>
This naming is consistent with bazel's `platforms` which names
high-level cpu architectures but not architecture details or cpu
features.

Signed-off-by: Chris Frantz <cfrantz@google.com>
@cfrantz cfrantz merged commit f7299c2 into lowRISC:riscv32 Aug 3, 2021
cfrantz added a commit to cfrantz/bazel-embedded that referenced this pull request Apr 8, 2022
)

Initial toolchain definition for riscv32

1. Configure lowrisc_toolchain_rv32imc for use by the OpenTitan project.
2. Deliver per-device flags to the GccEmbeddedFeatures function.
3. Turn off some per-feature compiler/linker flags.  Need to find a
   better way to do this.

Signed-off-by: Chris Frantz <cfrantz@google.com>
cfrantz added a commit to cfrantz/bazel-embedded that referenced this pull request Apr 8, 2022
)

Initial toolchain definition for riscv32

1. Configure lowrisc_toolchain_rv32imc for use by the OpenTitan project.
2. Deliver per-device flags to the GccEmbeddedFeatures function.
3. Turn off some per-feature compiler/linker flags.  Need to find a
   better way to do this.

Signed-off-by: Chris Frantz <cfrantz@google.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
3 participants