This is an archive of the discontinued LLVM Phabricator instance.

[VP] Add more functional SD opcodes to definitions
ClosedPublic

Authored by luke on Apr 17 2023, 6:07 AM.

Details

Summary

This defines more equivalent base SD opcodes for various VP nodes, so
that getVPForBaseOpcode can do more lookups of VP-equivalent operations.

Diff Detail

Event Timeline

luke created this revision.Apr 17 2023, 6:07 AM
Herald added a project: Restricted Project. · View Herald TranscriptApr 17 2023, 6:07 AM
Herald added subscribers: asb, pmatos. · View Herald Transcript
luke requested review of this revision.Apr 17 2023, 6:07 AM
Herald added a project: Restricted Project. · View Herald TranscriptApr 17 2023, 6:07 AM
This revision is now accepted and ready to land.Apr 17 2023, 9:56 AM

Is this really NFC though? Might the DAGCombiner not be able to match more patterns to fma with this information?

// fold (fadd (fmul x, y), z) -> (fma x, y, z)
if (isContractableFMUL(N0) && (Aggressive || N0->hasOneUse())) {
  return matcher.getNode(PreferredFusedOpcode, SL, VT, N0.getOperand(0),
                         N0.getOperand(1), N1);

With knowledge about FMUL -> VP_FMUL, this code might now trigger? I'm not 100% familiar with this matcher code so it's just a hunch.

It would be good to double-check this, add tests, and/or remove "NFC" from the description.

luke added a comment.Apr 17 2023, 12:42 PM

Is this really NFC though? Might the DAGCombiner not be able to match more patterns to fma with this information?

// fold (fadd (fmul x, y), z) -> (fma x, y, z)
if (isContractableFMUL(N0) && (Aggressive || N0->hasOneUse())) {
  return matcher.getNode(PreferredFusedOpcode, SL, VT, N0.getOperand(0),
                         N0.getOperand(1), N1);

With knowledge about FMUL -> VP_FMUL, this code might now trigger? I'm not 100% familiar with this matcher code so it's just a hunch.

It would be good to double-check this, add tests, and/or remove "NFC" from the description.

Good point, it’s definitely NFC-intended but will investigate this and add a test

luke updated this revision to Diff 514606.Apr 18 2023, 4:52 AM

Remove NFC and update commit message to explain why

luke retitled this revision from [VP] Add more functional SD opcodes to definitions. NFC to [VP] Add more functional SD opcodes to definitions.Apr 18 2023, 4:52 AM
luke added a comment.EditedApr 18 2023, 4:57 AM

So from what I can tell, VPMatchContext can now make VP nodes from FP_EXTEND and FMAD. However there's no observable change because with the former, TLI.isFPExtFoldable returns false on RISC-V, and with the latter it's explicitly disabled with a FIXME to re-enable it at some point.
But I've removed the NFC anyway: I believe these nodes are equivalent so these folds should be correct if they are ever enabled.

So from what I can tell, VPMatchContext can now make VP nodes from FP_EXTEND and FMAD. However there's no observable change because with the former, TLI.isFPExtFoldable returns false on RISC-V, and with the latter it's explicitly disabled with a FIXME to re-enable it at some point.
But I've removed the NFC anyway: I believe these nodes are equivalent so these folds should be correct if they are ever enabled.

Thanks for checking.

This revision was landed with ongoing or failed builds.Apr 28 2023, 2:19 AM
This revision was automatically updated to reflect the committed changes.