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

[Brandon Wu via Phabricator via llvm-commits]

4vtomat accepted this revision.
4vtomat added a comment.

LGTM



================
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>;
----------------
asb wrote:
> ego wrote:
> > 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.
> > 
> > 
> We often do add additioanl space before the `:` even though it has the disadvantage you mention of risking NFC changes if new identifiers are added. It has the advantage that when scanning definitions inheriting from the same class, the differences in class parameters are easier to spot as they're aligned (e.g. the changing bit pattern). You'll see examples of this e.g. in RISCVInstrInfoF.td
> 
> ```
> let SchedRW = [WriteFMA32, ReadFMA32, ReadFMA32, ReadFMA32] in {
> defm FMADD_S  : FPFMA_rrr_frm_m<OPC_MADD,  0b00, "fmadd.s",  FINX>;
> defm FMSUB_S  : FPFMA_rrr_frm_m<OPC_MSUB,  0b00, "fmsub.s",  FINX>;
> defm FNMSUB_S : FPFMA_rrr_frm_m<OPC_NMSUB, 0b00, "fnmsub.s", FINX>;
> defm FNMADD_S : FPFMA_rrr_frm_m<OPC_NMADD, 0b00, "fnmadd.s", FINX>;
> }
> ```
> But I don't think we're fully consistent with alignment style within lib/Target/RISCV so this isn't something I'd split hairs over.
I think any alignment style is fine if it is consistent in a single file lol~
Thank you for your change!


================
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>;
----------------
ego wrote:
> 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.
You can skip this~


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

https://reviews.llvm.org/D148483