For most fp16 vector ops, we could promote it to fp32 vector when zvfhmin is enable but zvfh is not.
But for nxv32f16, we need to split it first since nxv32f32 is not a valid MVT.
Details
Diff Detail
- Repository
- rG LLVM Github Monorepo
Event Timeline
llvm/lib/Target/RISCV/RISCVISelLowering.cpp | ||
---|---|---|
1153 | Use isTypeLegal? | |
4926 | Does std::tie(LoOperands[j], HiOperands[j]) = work? | |
5364 | Drop this change | |
5920 | Can we make a new function in place of lowerToScalableOp that does this check so we don't repeat it so many times. | |
6007 | Can we make a new function in place of lowerVPOp that encapsulates this check? | |
13781 | Can this be moved into performVFMADD_VLCombine? |
llvm/lib/Target/RISCV/RISCVISelLowering.cpp | ||
---|---|---|
13781 | This change is done in parent revision https://reviews.llvm.org/D151414. |
llvm/lib/CodeGen/SelectionDAG/LegalizeVectorOps.cpp | ||
---|---|---|
169 | nit: can you add a docstring? | |
579 | NewVecVT name implies that it is already a vector type. Do you need to check it here? Would it be better to assert right after you assign to NewVecVT that it isVector? | |
580 | NewVecVT.getVectorElementType().isFloatingPoint() -> NewScalarVT.isFloatingPoint()? | |
580 | Does it matter if NewVecTy isVector? Does it matter if NewVecVT.getVectorElementType().isFloatingPoint()? If all we're worried about is promoting a f16 operand to a f32 operand, I think we may be able to just check if the operand isFP, and bitcast otherwise. | |
585 | Why BITCAST instead of ANY_EXTEND, but for the scalar below we ANY_EXTEND? | |
586 | I think that the start_value is always the 0th argument. I tested this by instrumenting the code with assert(j ==0) when this case was true. Maybe the 0th argument (this one that triggers this case) can be handled outside the loop to simplify this nested if branch, and the loop can start at 1? | |
661 | I think you can drop surrounding () here. | |
llvm/lib/Target/RISCV/RISCVISelLowering.cpp | ||
831 | are there any more you plan on adding to this patch? | |
944 | custom -> Custom. Can you also clarify in comment that the reason we need to split is because promotion would lead to LMUL16 without splitting, but we don't have LMUL16? |
llvm/lib/CodeGen/SelectionDAG/LegalizeVectorOps.cpp | ||
---|---|---|
585 | Part of this code is from the function VectorLegalizer::Promote, so there is some cases such as promote AND v2i32 to v1i64 in x86, so I set bitcast here to keep consistent with VectorLegalizer::Promote. But thanks for your comment, I rethink about the promotion of reduce op. I think there won't be any cases to use BITCAST now, so I rewrite this fucntion. |
llvm/lib/Target/RISCV/RISCVISelLowering.cpp | ||
---|---|---|
394 | Does this need to be fixed as a part of this patch? | |
5509–5527 | Why don't we reuse the logic below? // Widening if (IsInt2FP) { // Do a regular integer sign/zero extension then convert to float. // Narrowing if (IsInt2FP) { // One narrowing int_to_fp, then an fp_round. narrowing int_to_fp then an fp_round seems to be what we're doing here right? | |
6037–6055 | Similar question, is there a good reason to not have this in lowerVPFPIntConvOp? |
llvm/lib/Target/RISCV/RISCVISelLowering.cpp | ||
---|---|---|
394 | I don't think so this part should be involved into this patch. This fixme is about bf16 not fp16. | |
5509–5527 | Thanks for advice. I think it would make the logic a little diffcult to read. If we want to reuse this function, we should merge it into this scope. if (SrcEltSize > (2 * EltSize)) { if (IsInt2FP) { } } But actually we not only have i64->f16 but also i32 i16 and even i8 ->f16, the condition of this if statement will become complict. if (SrcEltSize > (2 * EltSize) || ZvfhminPromote) { if (IsInt2FP) { } } And the other part that lower FP2Int should also should exculde ZvfhminPromote logic. |
LGTM.
llvm/lib/Target/RISCV/RISCVISelLowering.cpp | ||
---|---|---|
5509–5527 |
I will defer to you to decide what you think is best. If you think it will make it more difficult to read, I am happy to keep as is. I only brought it up because I do not see [[fallthrough]] used very often. But since it leads to easier to read code, I support your decision. |
nit: can you add a docstring?