Page MenuHomePhabricator

[SVE][CodeGen] Lower scalable fp_extend & fp_round operations
ClosedPublic

Authored by kmclaughlin on Sep 25 2020, 10:06 AM.

Details

Summary

This patch adds FP_EXTEND_MERGE_PASSTHRU & FP_ROUND_MERGE_PASSTHRU
ISD nodes, used to lower scalable vector fp_extend/fp_round operations.
fp_round has an additional argument, the 'trunc' flag, which is an integer of zero or one.

This also fixes a warning introduced by the new tests added to sve-split-fcvt.ll,
resulting from an implicit TypeSize -> uint64_t cast in SplitVecOp_FP_ROUND.

Diff Detail

Event Timeline

kmclaughlin created this revision.Sep 25 2020, 10:06 AM
kmclaughlin requested review of this revision.Sep 25 2020, 10:06 AM
sdesmalen added inline comments.Sep 25 2020, 10:50 AM
llvm/lib/Target/AArch64/AArch64ISelLowering.cpp
15926

is this change intentional?

llvm/lib/Target/AArch64/AArch64SVEInstrInfo.td
1451

This matches both 0 and 1, but is otherwise ignored in this pattern.
I wonder if this should match only 0 for normal rounding?

llvm/lib/Target/AArch64/AArch64ISelLowering.cpp
15926

This is to account for the extra non-vector argument, with the original assert being overly restrictive.

llvm/lib/Target/AArch64/AArch64SVEInstrInfo.td
1451

I don't believe the "extra" operand affects the functional requirements of the node, so it's not the case that you need to select "normal rounding" or something else. It looks to be purely an optimisation aid and I don't know why it's not a flag like isExact(), which can be attached to things like shifts.

paulwalker-arm accepted this revision.Sep 29 2020, 2:46 AM

FP_ROUND's extra operand is a tad inconvenient but that's not your fault and you're following the naming rules for _MERGE_PASSTHRU so this LGTM. I'll note there's no illegal to illegal test for fptrunc like there is for fpext but then I don't know what those tests are verifying that's not already covered by the illegal<->legal tests.

This revision is now accepted and ready to land.Sep 29 2020, 2:46 AM
sdesmalen accepted this revision.Sep 29 2020, 8:44 AM

LGTM as well.

llvm/lib/Target/AArch64/AArch64SVEInstrInfo.td
1451

Okay, I guess that's fine then. I see the fpround SDNode also doesn't reflect that operand.

def SDTFPRoundOp  : SDTypeProfile<1, 1, [   // fround
  SDTCisFP<0>, SDTCisFP<1>, SDTCisOpSmallerThanOp<0, 1>, SDTCisSameNumEltsAs<0, 1>
]>;
def fpround    : SDNode<"ISD::FP_ROUND"   , SDTFPRoundOp>;

nit: @kmclaughlin could you maybe just add a comment that the operand for the 'precise' flag, matched by (i64 timm0_1), is ignored?

This revision was landed with ongoing or failed builds.Oct 1 2020, 4:18 AM
This revision was automatically updated to reflect the committed changes.
kmclaughlin marked an inline comment as done.