This is an archive of the discontinued LLVM Phabricator instance.

[Matrix] Preserve existing fast-math flags during lowering
ClosedPublic

Authored by effective-light on May 27 2021, 2:47 AM.

Details

Summary

This patch makes it so, floating-point instructions created in
LowerMatrixIntrinsics retain fast-math flags from instructions that are
higher up the chain.

Fixes https://bugs.llvm.org/show_bug.cgi?id=49738

Diff Detail

Event Timeline

effective-light requested review of this revision.May 27 2021, 2:47 AM
Herald added a project: Restricted Project. · View Herald TranscriptMay 27 2021, 2:47 AM
fhahn added a comment.May 27 2021, 3:03 AM

Thanks for the patch! Could you add test coverage for lowering of the other FP instructions (fsub, fmul, fneg)? And maybe also with some variations of individual FMFs.

llvm/lib/Transforms/Scalar/LowerMatrixIntrinsics.cpp
1154

I think it would be cleaner to just pass in the FMF object here? We don't need the instruction for anything else.

Also, I think we don't need to separately pass in AllowContraction any longer?

llvm/test/Transforms/LowerMatrixIntrinsics/preserve-existing-fast-math-flags.ll
16

Is there a reason we need the insert element instructions here, rather than passing in the second operand as vector to the function as well?

Make existing tests conform to the changes.

anemet added inline comments.May 27 2021, 8:18 AM
llvm/lib/Transforms/Scalar/LowerMatrixIntrinsics.cpp
1154

+1

Pass FastMathFlags directly into emitMatrixMultiply and remove the
AllowContraction operator from emitMatrixMultiply

effective-light marked 2 inline comments as done.May 27 2021, 5:29 PM

Include more tests.

effective-light marked an inline comment as done.May 28 2021, 1:20 AM
fhahn added a comment.May 28 2021, 4:12 AM

Thanks for the update! A few more suggestions about dealing with the FMF object.

llvm/lib/Transforms/Scalar/LowerMatrixIntrinsics.cpp
407

Instead of returning None, could we just return a FastMathFlags object with all options set the false (which is the default) ?

1174

I think this should be the other way around, if AllowContractEnabled we could set AllowContraction on FMF. This default could be set in getFastMathFlags?

1686

Can we instead use the FMF object in those versions instead? AllowContraction option should probably also add the contract flag to those.

Restructure as suggested and fix more tests.

effective-light marked 3 inline comments as done.May 28 2021, 9:22 PM
fhahn accepted this revision.Jun 1 2021, 3:06 AM

LGTM with the inline nits. Thanks!

llvm/lib/Transforms/Scalar/LowerMatrixIntrinsics.cpp
410

nit: drop {}

llvm/test/Transforms/LowerMatrixIntrinsics/preserve-existing-fast-math-flags.ll
179

could you split up the test, possibly into separate ones for each multiply? Otherwise the IR to check for a single function seems unnecessarily big.

This revision is now accepted and ready to land.Jun 1 2021, 3:06 AM

Address nit and break up the test function.

effective-light marked 2 inline comments as done.Jun 1 2021, 3:59 AM
fhahn accepted this revision.Jun 1 2021, 4:28 AM

Thanks, still LGTM

@fhahn can you land this when you get the chance (I still don't have commit access)?

This revision was automatically updated to reflect the committed changes.
fhahn added a comment.Jun 3 2021, 7:31 AM

@fhahn can you land this when you get the chance (I still don't have commit access)?

Sure, thanks for the heads-up!