4vtomat marked 2 inline comments as done.
4vtomat added inline comments.
================
Comment at: clang/include/clang/Basic/riscv_vector.td:2378
+
+multiclass RVVOutBuiltinSetP<bit HasVV = 1, bit HasVS = 1, bit HasSigned = 0,
+ string type_range = "i", bit IsZvkb = 0> {
----------------
craig.topper wrote:
> What does `P` in this name mean? Is this because they use OP_P as their
> opcode? If so I don't think that should be part of how we name intrinsics.
Sure, I will rename by appending Zvk suffix for clarifying it's meaning~
================
Comment at: clang/include/clang/Basic/riscv_vector.td:2381
+ if HasVV then {
+ defvar suffix = !if(!or(HasVS, !eq(NAME, "vsm4r")), "vv", "v");
+ // We don't need suffix in Zvkb extension since it's consider as normal
----------------
michaelmaitland wrote:
> Why do we check `HasVS` when assigning suffix `vv`? I would have expected we
> use `HasVV`. In addition, why do we need to check `!eq(NAME, "vsm4r")`
> instead of setting the `HasVV` for that instruction?
We check `HasVS` when assigning suffix `vv` because when the instruction has
both `VV` and `VS` version, it uses double `v` as suffix, but if it just has
`VS` version, it will use single `v` as suffix, that's why we check if it also
has `VS` version in `HasVV` statement.
As per spec, the instruction name is vsm4r.v(single v), so it's an exception
for `HasVV` case.
================
Comment at: clang/include/clang/Basic/riscv_vector.td:2400
+ // mnemonics into its intrinsic function name.
+ defvar suffix = !if(!eq(NAME, "vgmul"), "vv", "vs");
+ defvar name = NAME # !if(!or(IsZvkb, !or(!eq(NAME, "vaesz"),
----------------
michaelmaitland wrote:
> Why not set `HasVS=1` and `HasVV=0` for `vaesz` instead of checking
> `!if(!eq(NAME, "vgmul"),...`?
>
> Also, do you mean to be discussing `vaesz` in the comment but use `vgmul`
> below?
The reason we set these condition to check `vaesz` and `vgmul` is because they
are exceptions.
They don't need suffix in their overloaded name since they only have one
version.
`HasVS=1` and `HasVV=0` is not enough to determine this situation.
================
Comment at: clang/include/clang/Basic/riscv_vector.td:2423
+ defm vandn : RVVIntBinBuiltinSet;
+ defm vbrev : RVVOutBuiltinSetP<1, 0, 1, "csil", 1>;
+ defm vbrev8 : RVVOutBuiltinSetP<1, 0, 1, "csil", 1>;
----------------
craig.topper wrote:
> I think I'd prefer to see the Zvbb instructions use their own class and not
> try to make `RVVOutBuiltinSetP` handle them and instructions that set HasVS.
It's a good idea, it will be much clearer and reduce the code size, thanks!
================
Comment at: clang/include/clang/Basic/riscv_vector.td:2449
+ let UnMaskedPolicyScheme = HasPassthruOperand in
+ defm vaeskf1 : RVVOutOp1BuiltinSet<"vaeskf1", "i", [["vi", "Uv", "UvUvUe"]]>;
+ defm vaeskf2 : RVVOutOp1BuiltinSetP<0>;
----------------
craig.topper wrote:
> I think the `Ue` needs a `K` since its required to be a constant? Also need
> to update SemaChecking to verify the valid range.
>
> I'm not sure whether we should use `Ue` or 'z' for constants.
I'm not sure what's the difference between `e` and `z`, but seems both of them
works fine.
However K is needed.
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D138810/new/
https://reviews.llvm.org/D138810
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
