This patch added the MC layer support of Zfinx extension.
Co-Authored-by: Shao-Ce Sun
achieveartificialintelligence on Dec 15 2020, 6:53 AM.Authored by
Firstly, please generate your diffs with full context (-U with a sufficiently-large number). Secondly, can we avoid having to do a bunch of duplication with some clever use of multiclasses for F/D/Zfh and pseudos? Though maybe it's small enough that the duplication is easier to reason about than an obfuscated abstracted version.
Also, do you not need more predicates? You can't just assume all of F, D and Zfh exist.
As for Zfinx itself, well, the idea is fine, but I really detest the way it's being done as an extension to F/D/Zfh. Running F code on an FZfh core _does not work_ so it is not an _extension_. Instead it should really be a set of separate extensions to I/E that conflict with F/D/Zfh, i.e. Zfinx, Zdinx and Zfhinx, but apparently asking code that complies with a ratified standard to change itself in order to not break when a new extension is introduced is a-ok in the RISC-V world.
Do you have implement register pair for rv32ifd_zfinx? I didn't saw the related implementation, but I could be wrong since I am not LLVM expert, in case you have implemented, you need a test case for that.
Your tests look like copies of the F/D/Zfh tests with not all the comments updated and instances of tests that just don't make sense for Zfinx. I only skimmed them and picked up a few issues, I haven't gone through them thoroughly, please do that yourself.
I started reviewing this alongside the specification in https://github.com/riscv/riscv-zfinx/blob/master/Zfinx_spec.adoc. At the time of writing, it seems to define "zfinx" but not "zfhinx" and "zfdinx" as seem to be used in this patch. I think intent is that rv32ifd_zfinx is the equivalent of "zfdinx" in this patch. Is there a reason to go for different naming, or a different version of the spec I should be looking at?
According to @jrtc27 's review that is
Ah I see. I interpreted jrtc27's comment as a general gripe about the spec (which perhaps could be relayed to those working on the zfinx spec) rather as a direction for changing this patch in particular. Anyway, it's a detail that shouldn't affect an initial review. Thanks for clarifying.
Well, it was "I'm uneasy about accepting a patch adding an extension that is fundamentally flawed in its current form" (unlike some of the others where they're subject to change but don't _break_ anything).