Depends on D141672
Details
Diff Detail
- Repository
- rG LLVM Github Monorepo
Event Timeline
llvm/lib/Target/RISCV/RISCVInstrInfoVPseudos.td | ||
---|---|---|
2831 | This parameter is never used. | |
2839 | I think this can use VPseudoBinaryV_VV | |
2841 | I think this can use VPseudoBinaryV_VX | |
4147 | I don't think using "PPat" instead of "VPat" meaningful highlights the difference between these patterns. The instructions being OP_P is an encoding detail only. |
llvm/lib/Target/RISCV/RISCVInstrInfoVPseudos.td | ||
---|---|---|
4325 | I think this might be the same as VPatTernaryNoMaskWithPolicy |
llvm/include/llvm/IR/IntrinsicsRISCV.td | ||
---|---|---|
299 | nit: destination vector type is the same as the source vector type | |
301 | From @craig.topper's comment https://reviews.llvm.org/D138810#inline-1494421
Can we drop P here too and reuse RISCVUnaryAAUnMasked? After adding DefaultAttrsIntrinsic it may be the same? If not, I have two questions:
| |
444 | Can you please add a docstring for this class? | |
1894 | IsVS should be renamed to HasVS to match class definition. Do you need to pass HasVS since it defaults to 1? | |
llvm/lib/Target/RISCV/RISCVInstrInfoVPseudos.td | ||
984 | Is there a reason behind using PPseudo instead of VPseudo? If its the same P as the other parts of this patch, should we keep it a suffix instead of prefix? | |
2672 | Docstring for MxListVF4 says for zext/sext.vf4. Are you able to update this docstring now that it has more uses? | |
4865 | I raise the same question about whether this should be VPatUnary as above and if P is needed, should it be suffixed? | |
6946 | It looks like this is part of the Vector Compress Instruction section. Should we add a vector crypto comment to separate? | |
7558 | It looks like this is part of the Vector Compress Instruction section. Should we add a vector crypto comment to separate? |
llvm/include/llvm/IR/IntrinsicsRISCV.td | ||
---|---|---|
301 | Yeah, I think you and Craig are right, I should change it to more descriptive name, thanks! | |
444 | Sure, I just added a comment above | |
1876 | Sure~ | |
1894 | For vgmul, class RISCVUnaryAAUnMaskedP<bit IsVS> And for the class definition, argument IsVS means: If this intrinsic is a VS intrinsic, then it will use llvm_anyvector_ty as it's 2nd input arg type and use LLVMMatchType<2> as 4th input arg type. However for multiclass definition, argument HasVS means, If this intrinsic has VS, then define one for it. | |
1894 |
| |
1901 | Oh, I forgot this part, thanks for reminding! | |
llvm/lib/Target/RISCV/RISCVInstrInfoVPseudos.td | ||
2672 | You are right, thanks! | |
6946 | Sure, thanks! |
llvm/test/CodeGen/RISCV/rvv/vwsll.ll | ||
---|---|---|
1459 | shift amounts should be iXLen like the non widening shift |
llvm/test/CodeGen/RISCV/rvv/vror.ll | ||
---|---|---|
2186 | Shift amounts should be iXLen. |
This update includes:
- Add the constraint: LMUL*VLEN < EGW during lowering instructions.
- Enumerate over all of lmul less or equal to vd type's lmul for those instructions that have scalar operand.
llvm/lib/Target/RISCV/RISCVISelDAGToDAG.cpp | ||
---|---|---|
1551 | This seems unnecessarily complex. https://reviews.llvm.org/D155439 already did this for non-intrinsics using an SDNodeXForm in tablegen | |
llvm/lib/Target/RISCV/RISCVISelLowering.cpp | ||
7121 | This is dead code in release builds and will give a build warning. | |
7261 | This assert isn't helpful to compiler users. It's not part of release builds. |
llvm/lib/Target/RISCV/RISCVInstrInfoZvk.td | ||
---|---|---|
846 | timm5 uses isInt<5>, but I think you want isUInt<5>? |
Resolved Craig's comment.
llvm/lib/Target/RISCV/RISCVISelDAGToDAG.cpp | ||
---|---|---|
1551 | Oh, that's much better, thanks! | |
llvm/lib/Target/RISCV/RISCVISelLowering.cpp | ||
7261 | Oh, ok. I just changed it to report_fatal_error~ | |
llvm/lib/Target/RISCV/RISCVInstrInfoVPseudos.td | ||
7570 | Oh, I forgot this instruction has different encoding of immediate, thanks! | |
llvm/lib/Target/RISCV/RISCVInstrInfoZvk.td | ||
846 | Oh, correct, thanks! |
llvm/lib/Target/RISCV/RISCVISelLowering.cpp | ||
---|---|---|
7123 | Doesn't this need to consider the value of vscale or VLEN? getKnownMinValue() always returns 64 for <vscale x 2 x i32>. | |
llvm/lib/Target/RISCV/RISCVInstrInfoZvk.td | ||
914 | NegImm64 no longer exists. I changed it in https://reviews.llvm.org/D156348 |
llvm/lib/Target/RISCV/RISCVISelLowering.cpp | ||
---|---|---|
7123 | My thought was: For each <vscale x N x M>, if N * M >= EGS * M == [4|8] * M, namely if N >= EGS, then it's valid. |
llvm/lib/Target/RISCV/RISCVISelLowering.cpp | ||
---|---|---|
7123 | <vscale x 4 x i32> is LMUL=2. I thought based on our conversation here https://github.com/riscv-non-isa/rvv-intrinsic-doc/pull/234#discussion_r1271843685 that we wanted to allow LMUL=1 with Zvl128b? |
llvm/lib/Target/RISCV/RISCVISelLowering.cpp | ||
---|---|---|
7123 | Oh, I misunderstood how LMUL is calculated, I thought it was VLEN / (N x M), so if zvl128b, <vscale x 4 x i32> would have LMUL=1. |
llvm/lib/Target/RISCV/RISCVISelLowering.cpp | ||
---|---|---|
7125 | Use RVVBitsPerBlock instead of 64 |
llvm/lib/Target/RISCV/RISCVISelLowering.cpp | ||
---|---|---|
7125 | Please add extra parentheses to Subtarget.getRealMinVLen() * VT.getSizeInBits().getKnownMinValue() / 64 to make the operator precedence clear |
llvm/lib/Target/RISCV/RISCVISelLowering.cpp | ||
---|---|---|
7125 | The reason I didn't add parentheses between VT.getSizeInBits().getKnownMinValue() / 64 is because if I do so, I need to case VT.getSizeInBits().getKnownMinValue() to either double or float since it could be fractional. Do you think it's better to do it this way? |
llvm/lib/Target/RISCV/RISCVISelLowering.cpp | ||
---|---|---|
7125 | Use (Subtarget.getRealMinVLen() * VT.getSizeInBits().getKnownMinValue()) / 64. It's the operator precedence the compiler will use for the current code. I just don't want to think about the operator precedence when reading the code. |
llvm/lib/Target/RISCV/RISCVISelLowering.cpp | ||
---|---|---|
7125 | Oh, I see~ |
llvm/lib/Target/RISCV/RISCVInstrInfo.td | ||
---|---|---|
252 | Yeah, you are right, let me have a patch to fix it, thanks! |
nit: destination vector type is the same as the source vector type