Page MenuHomePhabricator

Please use GitHub pull requests for new patches. Phabricator shutdown timeline

[RISCV] Model all 3 arithmetic sources of vector FMA at MC layer.
ClosedPublic

Authored by craig.topper on May 31 2023, 4:05 PM.

Details

Summary

For the most part, MC version of vector instructions don't model when
the destination is also a source. This primarily occurs for mask/tail
undisturbed. The MC layer can't see the policy bits so this kind of
makes sense.

We also lumped FMA instructions into this, but the destination of
FMA is an arithmetic source not just an undisturbed value. This needs
to be correct for llvm-mca to understand the dependency for the FMA
instructions. Though every other instruction is still wrong for
tail/mask undisturbed.

This patch models the FMA instructions correctly at the MCA layer.
This necessitates changes to the assembler to offset operand numbers.

We also have to remove hasMergeOp from the pseudo versions. This
property was originally for RISCVAsmPrinter to know to drop the
tail/mask undisturbed value when converting to the MC version. It's
been hijacked by the peepholes in RISCVISelDAGToDAG.cpp too so I had
to make some changes there. I'm going to look at ways to remove this
usage as a follow up.

I've added the extra sched class operand and fixed the operand order
for the scalar read class.

Diff Detail

Event Timeline

craig.topper created this revision.May 31 2023, 4:05 PM
Herald added a project: Restricted Project. · View Herald TranscriptMay 31 2023, 4:05 PM
craig.topper requested review of this revision.May 31 2023, 4:05 PM
Herald added a project: Restricted Project. · View Herald TranscriptMay 31 2023, 4:05 PM

I think cleaning up HasMergeOp makes sense. If we're now adding let HasMergeOp = 0 in various places it's telling me the design of our tablegen classes isn't working well.

llvm/lib/Target/RISCV/RISCVISelDAGToDAG.cpp
3190 ↗(On Diff #527223)

Do we need the extra parens on this first term (which was there before)?

llvm/lib/Target/RISCV/RISCVInstrInfoZvfbf.td
29

I wonder if it's worth having ExtraConstraints which are automatically appended to Constraints, to avoid us having to duplicate the "inherent" tied operand constraints that the base class should be setting.

I don't know how practical that is given our class hierarchies, just a thought. It might make things harder to understand, not easier.

craig.topper added inline comments.Jun 1 2023, 11:03 AM
llvm/lib/Target/RISCV/RISCVInstrInfoZvfbf.td
29

automatically appended in tablegen itself or somewhere in our class hierarchy?

Try to address review comments

More updates

Remove parentheses from RISCVISelDAGToDAG.cpp

frasercrmck added inline comments.Jun 2 2023, 12:31 AM
llvm/lib/Target/RISCV/RISCVInstrInfoV.td
1318–1319

Aren't these properties (including WidenV) inherent to VWMAC_MV_V_X and VWMAC_MV_X instructions? I wonder why we're not setting them in the classes themselves.

1421–1422

Same question about properties inherent to VWMAC_FV_V_F

llvm/lib/Target/RISCV/RISCVInstrInfoZvfbf.td
29

It wasn't very well fleshed out, sorry. I had something in mind using in our class hierarchy. I think what you've done in your latest update has assuaged most of my concerns. I'll say what I had in mind anyway though.

I was considering there being inside the base class let Constraints = !strconcat("@earlyclobber $vd_wb", ExtraConstraints) (which defaults to being empty), and then any defs like this one can do let ExtraConstraints = ", $vd = $vd_wb".

The notion of additive constraints makes sense to me, as I don't think changing or removing such things from the base classes is easy to follow, and repetition is more error-prone. But I think it's very specific to the class hierarchies whether this idea actually makes anything simpler or not.

Push WidenV constraint into class.
Finish using the EarlyClobber parameter I added to remove constraints.

@frasercrmck does this look ok now?

rogfer01 accepted this revision.Aug 4 2023, 12:50 AM

I could not test it fully as now some of the hunks can't be applied but the change LGTM.

llvm/lib/Target/RISCV/RISCVInstrInfoVPseudos.td
3081

I'm not sure I get this: were they never commutable in the first place due to their widening behaviour?

This revision is now accepted and ready to land.Aug 4 2023, 12:50 AM
craig.topper added inline comments.Aug 4 2023, 8:13 AM
llvm/lib/Target/RISCV/RISCVInstrInfoVPseudos.td
3081

I think this might be left over from when I added an extra parameter to the class so I had to list this explicitly. Now that there is no parameter, we don't need this change.

But I think we should make vector instructions commutable so MachineCSE can swap operands to find CSE opportunities.

This revision was landed with ongoing or failed builds.Aug 4 2023, 9:09 AM
This revision was automatically updated to reflect the committed changes.
wangpc added inline comments.Aug 4 2023, 9:33 AM
llvm/lib/Target/RISCV/RISCVInstrInfoV.td
625

We may not need comments for arguments since we have supported named arguments.