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

Add OR operator for option '--with-extra-multilib-test' #1378

Merged
merged 1 commit into from
Dec 11, 2023

Conversation

Incarnation-p-lee
Copy link
Contributor

This patch allows you provide the flags in build flags array acts on arch-abi respectively, you can use ',' to separate them. For example:

rv64gcv-lp64d:--param=riscv-autovec-lmul=dynamic,--param=riscv-autovec-preference=fixed-vlmax

will be consider as two target boards same as below:

riscv-sim/-march=rv64gcv/-mabi=lp64d/-mcmodel=medlow/--param=riscv-autovec-preference=fixed-vlmax
riscv-sim/-march=rv64gcv/-mabi=lp64d/-mcmodel=medlow/--param=riscv-autovec-lmul=dynamic

However, you can also leverage AND(:), OR(,) operator together but the OR(,) will always have the higher priority. For example:

rv64gcv-lp64d:--param=riscv-autovec-lmul=dynamic:--param=riscv-autovec-preference=fixed-vlmax,--param=riscv-autovec-lmul=m2

will be consider as tow target boars same as below:

riscv-sim/-march=rv64gcv/-mabi=lp64d/-mcmodel=medlow/--param=riscv-autovec-lmul=dynamic/--param=riscv-autovec-preference=fixed-vlmax
riscv-sim/-march=rv64gcv/-mabi=lp64d/-mcmodel=medlow/--param=riscv-autovec-lmul=m2

This patch allows you provide the flags in build flags array acts on arch-abi
__respectively__, you can use ',' to separate them. For example:

`rv64gcv-lp64d:--param=riscv-autovec-lmul=dynamic,--param=riscv-autovec-preference=fixed-vlmax`

will be consider as two target boards same as below:

```
riscv-sim/-march=rv64gcv/-mabi=lp64d/-mcmodel=medlow/--param=riscv-autovec-preference=fixed-vlmax
riscv-sim/-march=rv64gcv/-mabi=lp64d/-mcmodel=medlow/--param=riscv-autovec-lmul=dynamic
```

However, you can also leverage AND(`:`), OR(`,`) operator together but the
OR(`,`) will always have the higher priority. For example:

`rv64gcv-lp64d:--param=riscv-autovec-lmul=dynamic:--param=riscv-autovec-preference=fixed-vlmax,--param=riscv-autovec-lmul=m2`

will be consider as tow target boars same as below:

```
riscv-sim/-march=rv64gcv/-mabi=lp64d/-mcmodel=medlow/--param=riscv-autovec-lmul=dynamic/--param=riscv-autovec-preference=fixed-vlmax
riscv-sim/-march=rv64gcv/-mabi=lp64d/-mcmodel=medlow/--param=riscv-autovec-lmul=m2
```

Signed-off-by: Pan Li <[email protected]>
@cmuellner
Copy link
Collaborator

If I understood correctly, then this is not affecting the build, but only testing configurations.
If that's the case, then I wonder if we really need to make the interpretation of the --with-extra-multilib-test string more powerful, when we could simply add a mechanism that overloads the default behavior.

E.g. an environment variable EXTRA_MULTILIB_TEST could be used to override the current multilib test configuration. To test multiple such configurations, multiple invocations of make report with the appropriately set variable can be used. The calling context (e.g. a bash script, python script or a CI machinery) could then have all kinds of magic (of Turing complete languages) to generate the list and iterate over that.

I do not think this PR is targeting an invalid use case or is too complex, but I think it is not sufficient to express everything users may want to express to generate these multilib test lists. So we may end up with a PR introducing paratheses support in the future to overcome the limitations introduced by operator priorities...

@Incarnation-p-lee
Copy link
Contributor Author

If I understood correctly, then this is not affecting the build, but only testing configurations. If that's the case, then I wonder if we really need to make the interpretation of the --with-extra-multilib-test string more powerful, when we could simply add a mechanism that overloads the default behavior.

E.g. an environment variable EXTRA_MULTILIB_TEST could be used to override the current multilib test configuration. To test multiple such configurations, multiple invocations of make report with the appropriately set variable can be used. The calling context (e.g. a bash script, python script or a CI machinery) could then have all kinds of magic (of Turing complete languages) to generate the list and iterate over that.

I do not think this PR is targeting an invalid use case or is too complex, but I think it is not sufficient to express everything users may want to express to generate these multilib test lists. So we may end up with a PR introducing paratheses support in the future to overcome the limitations introduced by operator priorities...

Thank you for the comments. Actually, this PR comes from one end-user requirement from #1368, and the AND / OR is somehow idempotent with limited effect to support.

I am totally OK if there is an overall solution for this.

@cmuellner
Copy link
Collaborator

cmuellner commented Nov 28, 2023

You are right. Since this only affects the generate_target_board, the impact of adding new features is limited and merging this PR might be a reasonable choice.

@kito-cheng: any preferences?

@cmuellner cmuellner closed this Nov 28, 2023
@cmuellner
Copy link
Collaborator

cmuellner commented Nov 28, 2023

I reopened this PR since I closed it by accident.

@cmuellner cmuellner reopened this Nov 28, 2023
@Incarnation-p-lee
Copy link
Contributor Author

Thanks @cmuellner and never mind. Let's wait for @kito-cheng 's comments on this.

Copy link
Collaborator

@kito-cheng kito-cheng left a comment

Choose a reason for hiding this comment

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

LGTM, seems like I missed this somehow

@kito-cheng kito-cheng merged commit ffe927a into riscv-collab:master Dec 11, 2023
40 checks passed
@Incarnation-p-lee
Copy link
Contributor Author

Great, thanks @kito-cheng .

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