allow user to change bench options - riscv

[malus-brandywine]

DefaultBenchDeps is set to TRUE when benchmark suite is being built for
RiscV to enable configure benchmark options.

Signed-off-by: Nataliya Korovkina [email protected]

[axel-h]

Based on the comment #17 (comment), it seem the least we could add in the code is a more verbose comment. Is there a fundamental setup conflict now, that block running a custom benchmark set on RISC-V, which needs to be resolved?

[malus-brandywine]

The solution requires some other modifications. I'll take care of it within next few days.

[malus-brandywine]

As follow up of the #17 (comment) I checked the code.

Currently, event counters are supported only for HiFive, according to sel4bench.h

It means IPC bench which uses event counters will be built, but will NOT be functional for other platforms. IPC benchmark is ON by default.

Back to DefaultBenchDeps. Options depending on DefaultBenchDeps can be redefined in easy-settings.cmake and command line only if DefaultBenchDeps set to TRUE, otherwise benchmark options' values are set to local defaults.

Currently, the options depending on DefaultBenchDeps are only the ones that switch ON/OFF the whole corresponding application.

The patch will allow to manually pick up benchmark applications for RISC-V platforms, therefore to exclude IPC benchmark when needed.

[malus-brandywine]

The recent tests failed because the command line contained -DFAULT=TRUE for RISC-V.

With the option DefaultBenchDeps set to TRUE command line options make an effect now.

Should we change command line parameters in the test?

[axel-h]

• Style nitpicking: please change the commit description to: "bench/risc-v: allow user to change options"
• concerning the configuration, I'm a bit lost now. So the blocker is that the apps/fault benchmark app fails on RISC-V? This should become a separate issue then, which needs to be fixed. Once that is done, it could be made a know issue where a (temporary) work around is disabling this for RISC-V in the hw tests, so the fine grained user configurability from this PR can be merged.

[malus-brandywine]

• Style nitpicking: please change the commit description to: "bench/risc-v: allow user to change options"

I definitely will.

• concerning the configuration, I'm a bit lost now. So the blocker is that the apps/fault benchmark app fails on RISC-V? This should become a separate issue then, which needs to be fixed. Once that is done, it could be made a know issue where a (temporary) work around is disabling this for RISC-V in the hw tests, so the fine grained user configurability from this PR can be merged.

apps/fault never was supported for risc-v.
The tests that use init-build.sh with parameter -DFAULT=TRUE were successful because the parameter was ignored (DefaultBenchDeps was FALSE) and by default apps/fault is not built.

So, the used command line:
init-build.sh -DFASTPATH=TRUE -DHARDWARE=TRUE -DFAULT=TRUE -DRISCV64=TRUE -DPLATFORM=hifive
was misleading (apps/hardware is off by default as well).

I will open new issue about unsupported apps/fault #25 (comment)
(apps/hardware is successfully been built)

As for this PR, while apps/fault is not supported I would suggest to correct the command line for risc-v to:

init-build.sh -DFASTPATH=TRUE -DHARDWARE=TRUE -DRISCV64=TRUE -DPLATFORM=hifive

[axel-h]

Thanks, now I get it. Seem changing the CI workflow parameters can happen at these places:

• https://github.com/seL4/ci-actions/blob/master/sel4bench/builds.yml#L15
• https://github.com/seL4/ci-actions/blob/master/sel4bench/build.py#L27

[malus-brandywine]

Thank you, I'll fix it

[kent-mcleod]

Currently, the options depending on DefaultBenchDeps are only the ones that switch ON/OFF the whole corresponding application.

One of the configuration requirements of sel4bench is that it will only allow benchmarks to be configured and built that are supported. Otherwise someone who isn't intimately familiar with sel4bench can't tell that the benchmarks they've configured are unsupported. DefaultBenchDeps is sort of a big lock around several benchmarks that requires them to either be all supported by a platform, or none supported by a platform.

Removing the RISCV restriction should require that the minimum set of required benchmarks are now supported. It should then only be removed as a restriction only on the supported platforms that can successfully run these benchmarks.

[malus-brandywine]

In that case I will postpone the PR until the apps/fault is supported.

But I would suggest to remove parameters -DHARDWARE=TRUE -DFAULT=TRUE for HiFive from Performance webpage. For ones who aren't closely familiar with sel4bench it's misleading, they will not get those benchmarks anyway.

As well as from CI tests, to avoid confusion caused by difference of parameters here and there.

[axel-h]

@malus-brandywine Did you intend to close this or just accidently updated the master branch?

[malus-brandywine]

@axel-h I didn't drop the idea, but the commit has been postponed (prev. comment). I'm going to make another commit related to other task soon, it will go as another PR.

The changes I made in this commit are saved in a local branch. I'm going to revive them later.

[kent-mcleod]

But I would suggest to remove parameters -DHARDWARE=TRUE -DFAULT=TRUE for HiFive from Performance webpage. For ones who aren't closely familiar with sel4bench it's misleading, they will not get those benchmarks anyway.

As well as from CI tests, to avoid confusion caused by difference of parameters here and there.

This is a good idea

[lsf37]

I'll have a look at that.