This is an archive of the discontinued LLVM Phabricator instance.

[RISCV] Add isCommutable for pseudos without merge operand
ClosedPublic

Authored by wangpc on Aug 28 2023, 3:46 AM.

Details

Summary

vmadc and vector mask-register logical instructions are commutable
and there is no merge operand for their pseudos.

We add isCommutable=1 for these pseudos to gain more optimization
opportunities.

This patch fixes part of https://github.com/llvm/llvm-project/issues/64422.

Diff Detail

Event Timeline

wangpc created this revision.Aug 28 2023, 3:46 AM
Herald added a project: Restricted Project. · View Herald TranscriptAug 28 2023, 3:46 AM
wangpc requested review of this revision.Aug 28 2023, 3:46 AM
Herald added a project: Restricted Project. · View Herald TranscriptAug 28 2023, 3:46 AM

I'm not very familiar with how isCommutable works, but this change looks like a good idea as long as isCommutable only refers to the $rs2 and $rs1 operands, and does not say anything about commutability of $carry, $vl, or $sew.

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

The description says:

there is no merge operand for their pseudos

but it looks like we are forcing a merge operand. It looks like VPseudoBinaryV_VM defines VPseudoBinaryCarryIn which does not have a merge operand. Maybe we should not set forceMergeOpRead here?

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

typo: looks like we are forcing a merge operand SchedRead

wangpc edited the summary of this revision. (Show Details)Aug 28 2023, 8:42 AM
wangpc marked 2 inline comments as done.Aug 28 2023, 8:51 AM

I'm not very familiar with how isCommutable works, but this change looks like a good idea as long as isCommutable only refers to the $rs2 and $rs1 operands, and does not say anything about commutability of $carry, $vl, or $sew.

Yes. If we don't do anything else but set isCommutable to true, the first and second input operand will be commutable. For pseudos whose first operand is merge operand, we need to fix them in TargetInstrInfo::findCommutedOpIndices (I'm working on it).

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

Yes. I agree with you, it shouldn't be set.

craig.topper added inline comments.Aug 28 2023, 5:49 PM
llvm/lib/Target/RISCV/RISCVInstrInfoVPseudos.td
2264

Does this make PseudoVMANDN and PseudoVMORN commutable? That would be incorrect

wangpc updated this revision to Diff 554147.Aug 28 2023, 8:19 PM

Exclude PseudoVMANDN and PseudoVMORN.

wangpc marked an inline comment as done.Aug 28 2023, 8:20 PM
wangpc added inline comments.
llvm/lib/Target/RISCV/RISCVInstrInfoVPseudos.td
2264

Thanks! fixed!

This revision is now accepted and ready to land.Aug 28 2023, 8:24 PM
This revision was landed with ongoing or failed builds.Aug 29 2023, 12:52 AM
This revision was automatically updated to reflect the committed changes.
wangpc marked an inline comment as done.