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

Allow FPU flag override #471

Merged
merged 7 commits into from
Dec 4, 2019
Merged

Conversation

franksinankaya
Copy link
Contributor

Provide two environment variables as CLR_FPU_FLAGOVERRIDE to specify the VFP option in use such as vfpv3-d16 and CLR_FPU_CAPABILITY bitmask to tell which precision registers are actually implemented.

value of CLR_FPU_CAPABILITY is bit 0 for half, bit 1 for single and bit 2 for double precision.

@franksinankaya
Copy link
Contributor Author

@franksinankaya
Copy link
Contributor Author

Use

export CLR_FPU_CAPABILITY=0x3
export CLR_FPU_FLAGOVERRIDE="vfpv3-d16"

to build with vfpv3-d16 as an example

@janvorli
Copy link
Member

janvorli commented Dec 3, 2019

I don't like controlling the build scripts via environment variables. If we don't want to add an explicit option to our build scripts, I'd prefer using cmake properties instead. But it seems ok to add a build.sh option for this. @jkotas I am not sure what you really meant by your comment dotnet/coreclr#27890 (comment) that seems to make @franksinankaya opt for the env variable way.

@franksinankaya
Copy link
Contributor Author

I think the concern is we have too many ARMXYZ variants. I could bring the vfp options up all the way to build.sh instead of environment variables.

@jkotas
Copy link
Member

jkotas commented Dec 3, 2019

I agree that the environment variables are not the best way to do this.

We have a generic way to pass defines from the build command line to build. It is used to control number of features contributed by community that are not turned on in the default build. For example, dotnet/coreclr#13567

This should use the same scheme.

@franksinankaya
Copy link
Contributor Author

These feature flags don't seem to work in compiler detection stage in "configurecompiler.cmake"

@franksinankaya
Copy link
Contributor Author

should I reorder include(clrdefinitions.cmake) ? or create a clrcompilerdefinitions.cmake?

@janvorli
Copy link
Member

janvorli commented Dec 3, 2019

I am not sure I understand the problem. It think that you should be able to pass an extra cmake variabled using the -cmakeargs option (e.g. -cmakeargs -DCLR_FPU_CAPABILITY=0x7 -cmakeargs -DCLR_FPU_FLAGOVERRIDE=vfpv3) and then use these cmake variables instead of the env vars in your current implementation.

@franksinankaya
Copy link
Contributor Author

Cool, I wasn't aware of cmakeargs feature.

@franksinankaya
Copy link
Contributor Author

new build command for vfpv3-d16:

./build.sh -cross -arm -cmakeargs -DFEATURE_ARM_FPU_CAPABILITY=0x3 -cmakeargs -DFEATURE_ARM_FPU_TYPE=vfpv3-d16 -cmakeargs -DFEATURE_ARM_FPU_SPEC=1

@jkotas
Copy link
Member

jkotas commented Dec 3, 2019

Nit: The command line arguments for this do not have to have FEATURE_ prefix. It is odd to call these features.

@franksinankaya
Copy link
Contributor Author

new build command:

./build.sh -cross -arm -cmakeargs -DARM_FPU_CAPABILITY=0x3 -cmakeargs -DARM_FPU_TYPE=vfpv3-d16

@jkotas
Copy link
Member

jkotas commented Dec 3, 2019

Looks good to me. @janvorli ?

Copy link
Member

@janvorli janvorli left a comment

Choose a reason for hiding this comment

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

LGTM modulo the comments.

@franksinankaya
Copy link
Contributor Author

new build command:

./build.sh -cross -arm -cmakeargs -DCLR_ARM_FPU_CAPABILITY=0x3 -cmakeargs -DCLR_ARM_FPU_TYPE=vfpv3-d16

@franksinankaya
Copy link
Contributor Author

any clues on what these failures are? can we retrigger the failing tests?

@jkotas
Copy link
Member

jkotas commented Dec 4, 2019

This is flaky test. Retriggered.

@franksinankaya
Copy link
Contributor Author

It looks like it worked.

@jkotas jkotas merged commit 0c002cc into dotnet:master Dec 4, 2019
@BruceForstall
Copy link
Member

@franksinankaya Would it be useful to add some comments around your code changes here to explain how to invoke the build to use these new options? E.g., giving the example of

./build.sh -cross -arm -cmakeargs -DCLR_ARM_FPU_CAPABILITY=0x3 -cmakeargs -DCLR_ARM_FPU_TYPE=vfpv3-d16

and explain the bitmask? Or, maybe better, adding a small section to the document https://github.com/dotnet/runtime/blob/master/docs/workflow/building/coreclr/cross-building.md (or perhaps https://github.com/dotnet/runtime/blob/master/docs/workflow/building/coreclr/linux-instructions.md)?

@franksinankaya
Copy link
Contributor Author

@BruceForstall : Sure, I'll post a follow-up PR

@franksinankaya franksinankaya deleted the frkaya/vfpcustomize branch December 4, 2019 21:23
hopix pushed a commit to hopix/coreclr that referenced this pull request Feb 5, 2020
@ghost ghost locked as resolved and limited conversation to collaborators Dec 11, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants