Add ARC_MPY_OPTION to kconfig

[YuguoWH]

The CONFIG_ARC_HAS_ACCL_REGS is not set for most of ARC targets. When it is set, it provides saving & restoring to r58 and r59 which are acch and accl registers. The condition to set CONFIG_ARC_HAS_ACCL_REGS should be FPU and/or MPY > 6 but there's no corresponding kconfig item for MPY.
The issue mentioned in #39392 was caused by this, CONFIG_ARC_HAS_ACCL_REGS was not set but arcmwdt compiler uses accl and acch as general purpose registers which are corrupted during IRQ.
This PR add a new ARC_MPY_OPTION to kconfig to patch this problem.

Fixes #39392

[abrodkin]

I'd say this is a bit incomplete change as if we introduce that granular MPY option it makes sense to also handle that in options we pass to the compiler. And both: gcc & ccac (options are different for those). This also will potentially improve performance for platforms with powerful MPY.

[ruuddw]

Not sure I would like setting compiler options via kconfig defines. We have TCF files that have all the relevant options, I rather see these to be leading, and maybe generate a kconfig from that instead.

For now, why not simply define 'HAS_ACCL' for the relevant boards instead of introducing the MPY option?

[YuguoWH]

@ruuddw I think defining HAS_ACCL statically is not flexable for all arc targets. The help message for HAS_ACCL already states that it should be enabled when FPU and/or MPY > 6 but MPY is nowhere to be found. Defining MPY_OPTION has the same effect as define HAS_ACCL everywhere, but it can also provide more: We could add compiler options later so it could improve performance.

[ruuddw]

For selecting the relevant compiler flags, I'd like to see a move to using the ARC Tool Configuration files (TCF), instead of introducing corresponding kconfig options for all of these.
If you want to handle all that via kconfig, please do it completely as recommended by others already. If you don't want to do that in this PR, my preference would be to not introduce an 'MPY' option in kconfig now: less is more, no need to introduce config features now if you do not really need or use them now.

[evgeniy-paltsev]

Hmm, this looks very suspicious.

We set it for EMSK and GNU toolchain

zephyr_compile_options_ifdef(CONFIG_ARC_MPY_OPTION -mmpy-option=${CONFIG_ARC_MPY_OPTION})

but we hardcode number here.

[YuguoWH]

I'm taking reference from tcf files here but I find the option value in tcf file is mismatch between metaware option and gnu option. For example in emsk arcem9d.tcf there's -Xmpy_option=mpyd (which is synonym of -Xmpy_option=8) and -mmpy-option=6 at the same time. That's the reason I put hardcode here. What do you think if I change kconfig value to lowest mpy_option value in tcf (at the cost of disable some MPY instructions when use metaware toolchain)?

[YuguoWH]

I get the confirmation from GNU team that at the moment the EM mpy value is limited to 6 as DSP exensions for EM are not supported.

[evgeniy-paltsev]

Even if we actually need this I'd prefer to have this logic in some Cmake function/macro in arc .cmake and call it from SoC CMakeLists.txt. We can call it arc_finalize_mpy_compile_options and put option for both GNU and MWDT toolchain in it.

[ruuddw]

not sure - if we want a clean, generic solution, then I come back to my earlier suggestion: it would be better to have the option to use tcf files for all hardware specific compiler options/flags.

[abrodkin]

@ruuddw so then maybe we just set CONFIG_ARC_HAS_ACCL_REGS explicitly for all boards where those registers exists and decide which way to do its detection automatically (based on say .tcf or in any other way) later? That way we'll solve immediately that problem with corrupted registers.

[ruuddw]

@ruuddw so then maybe we just set CONFIG_ARC_HAS_ACCL_REGS explicitly for all boards where those registers exists and decide which way to do its detection automatically (based on say .tcf or in any other way) later? That way we'll solve immediately that problem with corrupted registers.

Yes, that's what I proposed as my first feedback to this PR already: "For now, why not simply define 'HAS_ACCL' for the relevant boards instead of introducing the MPY option?"

[YuguoWH]

I've run twister test with jenkins and the result proves the fix.

[ruuddw]

Thanks, this is much simpler and specific.

[evgeniy-paltsev]

BTW, the default FPU usage is without FPU_SHARING - we assume that we use FPU only in one thread, so we don't need ARC_HAS_ACCL_REGS if we have FPU by default (at least on ARC HS).

I guess we are enabling ARC_HAS_ACCL_REGS because we assuming that ACCL_REGS would be used by MPY instructions. Probably we need to add comment here that if we don't need to enable it if our system doesn't support MPY instructions / we are 100% sure that we don't use them.

[ruuddw]

Triggered by this, I checked and actually believe FPU doesn't always mean the accl registers are there. Instead, they are only available if fused multiply & add/sub is configured in the hardware.
I think this default line can simply be removed and left to SoC configuration.

[ruuddw]

See comments, I don't believe FPU implies by default that accumulator registers are present.

[ruuddw]

Simple is often better, fixing the 'CONFIG_HAS_ACCL_REGS was not set' can be as simple as setting it for the targets that need this.