This is an archive of the discontinued LLVM Phabricator instance.

[RISCV] Remove the _COMMUTABLE and _TA versions of FMA and wide FMA vector instructions.
ClosedPublic

Authored by craig.topper on Jul 21 2021, 5:16 PM.

Details

Summary

Use a tail policy operand instead. Inspired by the work in D105092,
but without the intrinsic interface changes.

Diff Detail

Event Timeline

craig.topper created this revision.Jul 21 2021, 5:16 PM
craig.topper requested review of this revision.Jul 21 2021, 5:16 PM
Herald added a project: Restricted Project. · View Herald TranscriptJul 21 2021, 5:16 PM
Herald added a subscriber: MaskRay. · View Herald Transcript

I think the summary/description should reference D105092?

If I've got this right, now all FMA pseudos are ostensibly "commutable" as far as LLVM's concerned, but we skip the optimization if the tail policy argument says otherwise?

I'm a bit concerned that LLVM has (at least in theory) the right to assume these instructions are commutable by checking the static MI.isCommutable() method without asking us about the tail policy. Maybe that's academic, I dunno.

craig.topper edited the summary of this revision. (Show Details)Jul 27 2021, 8:45 AM

I think the summary/description should reference D105092?

Yes. I guess I didn't copy the last digit.

If I've got this right, now all FMA pseudos are ostensibly "commutable" as far as LLVM's concerned, but we skip the optimization if the tail policy argument says otherwise?

Correct.

I'm a bit concerned that LLVM has (at least in theory) the right to assume these instructions are commutable by checking the static MI.isCommutable() method without asking us about the tail policy. Maybe that's academic, I dunno.

There's no guarantee that the first two operands are commutable. So I think any user of isCommutable() needs to call findCommutedOpIndices. X86 has checks for specific immediate values for vectors compares and vector shuffles in its implementation of findCommutedOpIndices.

frasercrmck accepted this revision.Jul 29 2021, 1:51 AM

I'm a bit concerned that LLVM has (at least in theory) the right to assume these instructions are commutable by checking the static MI.isCommutable() method without asking us about the tail policy. Maybe that's academic, I dunno.

There's no guarantee that the first two operands are commutable. So I think any user of isCommutable() needs to call findCommutedOpIndices. X86 has checks for specific immediate values for vectors compares and vector shuffles in its implementation of findCommutedOpIndices.

Fair enough, yeah. It's just a fairly unclear contract that's being entered into when you set some of these properties in TableGen, if you ask me. It's encouraging that we're not the only backend that would be affected if someone interpreted isCommutable as doing what it says on the tin.

LGTM.

This revision is now accepted and ready to land.Jul 29 2021, 1:51 AM
frasercrmck added inline comments.Jul 29 2021, 1:52 AM
llvm/lib/Target/RISCV/MCTargetDesc/RISCVBaseInfo.h
80

Oh, maybe this could be commented as the others are.

This revision was landed with ongoing or failed builds.Aug 4 2021, 10:40 AM
This revision was automatically updated to reflect the committed changes.