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

[Alex Bradbury via Phabricator via llvm-commits]

asb added a comment.

In D148483#4282342 <https://reviews.llvm.org/D148483#4282342>, @ego wrote:

> I do not have commit access. I propose that, if everything looks good, I'll upload a rebased version and let someone (Alex?)  commit on my behalf?
> Let me know if there is a better way.

Yep, that's the usual way. Just indicate when it's ready for commit from your perspective and confirm the name + email address and one of us can commit. Whoever is around at the time and has been following this patch really - I believe we're in a different timezone but I'll happily commit if no-one beats me to it when you indicate it's ready.

See https://llvm.org/docs/DeveloperPolicy.html#obtaining-commit-access for details on obtaining commit access yourself - I'd suggest you email Chris after another landing another couple patches or so.

Additional changes LGTM from my perspective.



================
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>;
----------------
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.


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

https://reviews.llvm.org/D148483