Page MenuHomePhabricator

[Matrix] Use nuw/nsw operand bundles for matrix.multiply.
Needs ReviewPublic

Authored by fhahn on Jun 4 2020, 8:04 AM.

Details

Summary

This patch updates LowerMatrixIntrinsics to actually use the nuw/nsw
bundles during code generation.

Using operand bundles for those optional arguments seems a bit more
elegant and lightweight than making matrix.multiply variadic (as
proposed in D80124). It should also be easier to add additional
arguments, like the layout of the operands, in the future.

There seems to be some precedence of using operand bundles in that way.
For example, recently the statepoint intrinsics are transitioning to use
operand bundles. See D80598 and 27304b1737a3 for examples.

Diff Detail

Event Timeline

fhahn created this revision.Jun 4 2020, 8:04 AM
Herald added a project: Restricted Project. · View Herald TranscriptJun 4 2020, 8:04 AM
fhahn added a subscriber: reames.Jun 4 2020, 8:06 AM

cc @reames as he recently worked on making use of operand bundles for the state point intrinsics and may have some suggestions on the use of bundles.

fhahn updated this revision to Diff 273758.Jun 26 2020, 9:19 AM

Rebased & ping :)

xbolva00 added inline comments.
llvm/lib/IR/LLVMContext.cpp
83 ↗(On Diff #273758)

Copy paste error

Seems reasonable to me.

fhahn marked 2 inline comments as done.Jun 29 2020, 10:53 AM
fhahn added inline comments.
llvm/lib/IR/LLVMContext.cpp
83 ↗(On Diff #273758)

Fixed, thanks!

fhahn updated this revision to Diff 274182.Jun 29 2020, 10:53 AM
fhahn marked an inline comment as done.

Fix copy-paste error, thanks @xbolva00!

reames added a comment.Mon, Jul 6, 5:13 PM

(drive by comments only, please don't block on me)

Reading over this, I find myself wondering if this is actually matrix specific. Would it make sense to have a means to declare the operations in an reduce.add are nsw/nuw?

As a semantic clarification, does the nsw/nuw markers on the matrix make any assumptions about order of operations?

llvm/docs/LangRef.rst
2268 ↗(On Diff #274182)

The LangRef text is not sufficient to describe the syntax. I know this only because I went and read your verifier changes. Please clarify, ideally with an example.

fhahn planned changes to this revision.Wed, Jul 15, 9:41 AM

(drive by comments only, please don't block on me)

Thanks, that's very much appreciated!

Reading over this, I find myself wondering if this is actually matrix specific. Would it make sense to have a means to declare the operations in an reduce.add are nsw/nuw?

Not really, it would could be used by other intrinsics as well. I'll try to generalize it.

As a semantic clarification, does the nsw/nuw markers on the matrix make any assumptions about order of operations?

I don't think the order of operations should matter, because the order we sum the results of the multiplications should not impact whether we overflow or not I think.

fhahn updated this revision to Diff 279344.Mon, Jul 20, 2:09 PM

Split off LangRef/Verifier changes to D84201 and generalize them by dropping the matrix_ prefix.

fhahn marked an inline comment as done.Mon, Jul 20, 2:26 PM
fhahn added inline comments.
llvm/docs/LangRef.rst
2268 ↗(On Diff #274182)

In the updated, generalized version I provided a more detailed specification and an example (D84201). I hope this makes things clearer.

fhahn retitled this revision from [Matrix] Add matrix_nuw/matrix_nsw operand bundles for matrix.multiply. to [Matrix] Use nuw/nsw operand bundles for matrix.multiply..Mon, Jul 20, 2:27 PM
fhahn edited the summary of this revision. (Show Details)