This is an archive of the discontinued LLVM Phabricator instance.

[RISCV] Apply promotion for f16 vector ops when only have zvfhmin
ClosedPublic

Authored by jacquesguan on Jun 27 2023, 2:34 AM.

Details

Summary

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.

Diff Detail

Event Timeline

jacquesguan created this revision.Jun 27 2023, 2:34 AM
Herald added a project: Restricted Project. · View Herald TranscriptJun 27 2023, 2:34 AM
jacquesguan requested review of this revision.Jun 27 2023, 2:34 AM
Herald added a project: Restricted Project. · View Herald TranscriptJun 27 2023, 2:34 AM
craig.topper added inline comments.Jun 27 2023, 10:21 AM
llvm/lib/Target/RISCV/RISCVISelLowering.cpp
1134

Use isTypeLegal?

4731

Does std::tie(LoOperands[j], HiOperands[j]) = work?

5111

Drop this change

5644

Can we make a new function in place of lowerToScalableOp that does this check so we don't repeat it so many times.

5727

Can we make a new function in place of lowerVPOp that encapsulates this check?

13371

Can this be moved into performVFMADD_VLCombine?

Address comment.

jacquesguan marked 6 inline comments as done.Aug 2 2023, 1:18 AM
jacquesguan added inline comments.
llvm/lib/Target/RISCV/RISCVISelLowering.cpp
13371

This change is done in parent revision https://reviews.llvm.org/D151414.

jacquesguan marked an inline comment as done.

Add widen instruction test cases.

michaelmaitland added inline comments.Aug 7 2023, 3:31 PM
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
824

are there any more you plan on adding to this patch?

936

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?

Address comment.

jacquesguan marked 7 inline comments as done.Aug 8 2023, 1:35 AM
jacquesguan added inline comments.
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.

jacquesguan marked an inline comment as done.

rebase

support int_to_fp ops

llvm/lib/Target/RISCV/RISCVISelLowering.cpp
378–379

Does this need to be fixed as a part of this patch?

5254

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?

5759

Similar question, is there a good reason to not have this in lowerVPFPIntConvOp?

jacquesguan added inline comments.Aug 22 2023, 12:20 AM
llvm/lib/Target/RISCV/RISCVISelLowering.cpp
378–379

I don't think so this part should be involved into this patch. This fixme is about bf16 not fp16.

5254

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.
So should we trade servel lines of code with clear logic?

michaelmaitland accepted this revision.Aug 22 2023, 6:28 PM

LGTM.

llvm/lib/Target/RISCV/RISCVISelLowering.cpp
5254

I think it would make the logic a little diffcult to read. So should we trade servel lines of code with clear logic?

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.

This revision is now accepted and ready to land.Aug 22 2023, 6:28 PM
jacquesguan retitled this revision from [RISCV] Apply promotion for f16 vector ops when only have zvfhmin. to [RISCV] Apply promotion for f16 vector ops when only have zvfhmin.Aug 22 2023, 8:03 PM