[PATCH] D148483: RISC-V Zvk (vector crypto) specification update to 0.5.1 (Zvbb/Zvbc/Zvkt/Zvkng/Zvksg)

[Eric Gouriou via Phabricator via llvm-commits]

ego added inline comments.


================
Comment at: llvm/lib/Support/RISCVISAInfo.cpp:955
+                                        "zvkt"};
+static const char *ImpliedExtsZvksg[] = {"zvks", "zvkg"};
 static const char *ImpliedExtsXsfvcp[] = {"zve32x"};
----------------
4vtomat wrote:
> Maybe this should be in alphabetical order?
Sure, I'll send a follow-up to sort this series of constants and do the other proposed NFC changes.


================
Comment at: llvm/lib/Target/RISCV/RISCVFeatures.td:500
     : SubtargetFeature<"experimental-zvkg", "HasStdExtZvkg", "true",
-                       "'Zvkg' (Vector GCM instructions for Cryptography.)">;
+                       "'Zvkg' (Vector GCM instructions for Cryptography)">;
 def HasStdExtZvkg : Predicate<"Subtarget->hasStdExtZvkg()">,
----------------
4vtomat wrote:
> I guess this removal is NFC and not related to this patch~
Indeed, I should not have done this change in this commit.
I'll propose those changes in a separate PR/review/commit to decouple them from the Zvk update.


================
Comment at: llvm/lib/Target/RISCV/RISCVInstrInfoZvk.td:138-139
+  def VREV8_V  : VALUVs2<0b010010, 0b01001, OPMVV, "vrev8.v">;
+
+  defm VANDN_V : VALU_IV_V_X<"vandn", 0b000001>;
+  defm VROL_V  : VALU_IV_V_X<"vrol", 0b010101>;
----------------
4vtomat wrote:
> Alphabetical order, thanks!
> and remove blank line!
It is (was) sorted, the order was due to 'def' vs. 'defm'.

Would you rather have it sorted by identifier (VANDN / VBREV8 / VBREV / VC* / etc.)? That's certainly doable but requires slightly more involved sorting than pure line sorting in an editor.

I feel that there are multiple orders that make sense, and each of them make the most sense in different circumstances:
 - by encoding (0b01000, ...)
 - by identifier (VANDN, ...), or by mnemonic (usually the same order, but doesn't have to be)
 - by "logical grouping" (e.g., VANDN, VCLZ/VCTZ, VCPOP, VBREV8/VBREV/VREV, VROL/VROR, VWSLL), but those are quite subjective
 - by sorting the lines, which will group "def" and "defm" separately.

For me the line sorting was the simplest, but it's easy enough to sort by the identifier( ctrl-u 2 M-x sort-fields in Emacs). I realize now that this is what your patch did for other extensions, so I'm happy to give it a try.

I feel that if we sort by identifier, it make sense to align the identifier, inserting a space to align "def" and "defm" identifiers. I checked the style guide and did not find an explicit recommendation against that additional vertical alignment. We don't want to align the ':' as it would force re-alignment if we introduce a larger identifier, forcing NFC changes in order to maintain that vertical alignment.

Let me know what you think.




================
Comment at: llvm/lib/Target/RISCV/RISCVInstrInfoZvk.td:146
+let Predicates = [HasStdExtZvbc] in {
   defm VCLMUL_V : VCLMUL_MV_V_X<"vclmul", 0b001100>;
   defm VCLMULH_V : VCLMUL_MV_V_X<"vclmulh", 0b001101>;
----------------
4vtomat wrote:
> This also need indentation.
I do not understand this suggestion as I do not see missing indentation.
Are you suggesting to align the ':' ? In the comment above I argue against such alignment.
Please let me know.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D148483/new/

https://reviews.llvm.org/D148483