This is an archive of the discontinued LLVM Phabricator instance.

[RISCV] Move vfma_vl+fneg_vl matching to DAG combine.
ClosedPublic

Authored by craig.topper on Jun 21 2022, 4:34 PM.

Details

Summary

This patch adds 3 new _VL RISCVISD opcodes to represent VFMA_VL with
different portions negated. It also adds a DAG combine to peek
through FNEG_VL to create these new opcodes.

This is modeled after similar code from X86.

This makes the isel patterns more regular and reduces the size of
the isel table by ~37K.

The test changes look like regressions, but they point to a bug that
was already there. We aren't able to commute a masked FMA instruction
to improve register allocation because we always use a mask undisturbed
policy. Prior to this patch we matched two multiply operands in a
different order and hid this issue for these test cases, but a different
test still could have encountered it.

Diff Detail

Event Timeline

craig.topper created this revision.Jun 21 2022, 4:34 PM
Herald added a project: Restricted Project. · View Herald TranscriptJun 21 2022, 4:34 PM
craig.topper requested review of this revision.Jun 21 2022, 4:34 PM
Herald added a project: Restricted Project. · View Herald TranscriptJun 21 2022, 4:34 PM

Direction looks reasonable, a couple comments.

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

The logic flow here is very confusing. In particular, I can't quite tell if this handles the case where we invert both or not.

I'd suggest structuring this with an explicit if (NegMul && NegAcc) case, and then use early return.

As a further idea, what we really have here is for each case, pairs of inverted opcodes. Grouping the code that way would help readability.

llvm/lib/Target/RISCV/RISCVInstrInfoVVLPatterns.td
1242

It looks like after your change, these become regular across the four opcodes, can we use a pattern class here to reduce code duplication?

craig.topper added inline comments.Jun 23 2022, 11:38 AM
llvm/lib/Target/RISCV/RISCVISelLowering.cpp
8480

The logic flow here is very confusing. In particular, I can't quite tell if this handles the case where we invert both or not.

The first switch updates Opcode so the second switch will see the change.

I'd suggest structuring this with an explicit if (NegMul && NegAcc) case, and then use early return.

As a further idea, what we really have here is for each case, pairs of inverted opcodes. Grouping the code that way would help readability.

Is this an ask to reorder the cases in the switch so they are grouped in pairs or something else?

Reorder switch cases, add some comments. Use "and/or" instead of "or" in function description.
Add tablegen multiclass.

frasercrmck accepted this revision.Jun 23 2022, 11:26 PM

LGTM with a nit/question.

llvm/lib/Target/RISCV/RISCVInstrInfoVVLPatterns.td
1355

I took this comment to refer to the choice of opcode for a generic fma. If we've already decided the opcode we're doing a straight 1:1 mapping, so I don't think it holds?

This revision is now accepted and ready to land.Jun 23 2022, 11:26 PM
craig.topper added inline comments.Jun 23 2022, 11:32 PM
llvm/lib/Target/RISCV/RISCVInstrInfoVVLPatterns.td
1355

We could still pick VFMACC by rearranging the operands from the ISD node. But the ISD naming does imply a choice was already made. hmmm...

Drop comment. Rename multiclass Acc->Add.

This revision was landed with ongoing or failed builds.Jun 24 2022, 12:01 AM
This revision was automatically updated to reflect the committed changes.