This is an archive of the discontinued LLVM Phabricator instance.

[InstCombine] Test for matrix multiplication negation optimisation.
ClosedPublic

Authored by zjaffal on Sep 5 2022, 3:01 AM.

Details

Summary

If one of the operands is negated in a multiplication we can optimise the operation by moving the negation to the smallest operand or to the result

Diff Detail

Event Timeline

zjaffal created this revision.Sep 5 2022, 3:01 AM
Herald added a project: Restricted Project. · View Herald TranscriptSep 5 2022, 3:01 AM
zjaffal requested review of this revision.Sep 5 2022, 3:01 AM
Herald added a project: Restricted Project. · View Herald TranscriptSep 5 2022, 3:01 AM
zjaffal updated this revision to Diff 458463.Sep 7 2022, 8:28 AM

Add test case where fneg is used more than once

fhahn added a comment.Sep 8 2022, 12:50 AM

Thanks for the update! Could you also add quick comments to the test cases documenting the expected behavior? It would also good to have test cases with chains of matrix multiplies, where some arguments are negated. They don't need to be optimized by the first patch, but it would be good to make sure we do not crash on such cases (or run into an instcombine cycle)

zjaffal updated this revision to Diff 458868.Sep 8 2022, 2:21 PM

Add more test casses to address the following:

  1. Multiple matrix multiply operation
  2. Negation on both operands
  3. Negation on the result
fhahn added inline comments.Sep 9 2022, 1:59 AM
llvm/test/Transforms/InstCombine/matrix-multiplication-negation.ll
2

nit: is UTC_ARGS: --force-update really needed?

5

could you also add versions of test_negation_move_to_result & test_negation_not_moved where the second operand is negated instead?

16

Could also add another version of the test where the negation is moved from the second to the first operand?

27

Could you add quick comments to the tests saying why the negation should or should not be moved?

69

%b.neg isn't used?

90

AFAICT there's no case at the moment where it would be profitable to move the negation with chains, right? Could you add it?

zjaffal added inline comments.Sep 9 2022, 2:48 AM
llvm/test/Transforms/InstCombine/matrix-multiplication-negation.ll
5

Okay I will add those tests now

16

I think this is a good test case as well

69

I will add it now

zjaffal updated this revision to Diff 459014.Sep 9 2022, 4:32 AM

Add test cases for second operand and chain multiplication

fhahn added inline comments.Sep 9 2022, 4:43 AM
llvm/test/Transforms/InstCombine/matrix-multiplication-negation.ll
41

nit: %b should be %a?

65

s so the negation can be moved there -> should be not moved?

zjaffal updated this revision to Diff 459018.Sep 9 2022, 5:16 AM
zjaffal marked an inline comment as done.

Add tests to check for fast flags

zjaffal updated this revision to Diff 459165.Sep 9 2022, 12:38 PM

Add more test cases where operands are negated

zjaffal updated this revision to Diff 459449.Sep 12 2022, 7:02 AM

Add tests cases where we have both operands negated

zjaffal marked 7 inline comments as done.Sep 12 2022, 7:02 AM
zjaffal updated this revision to Diff 459686.Sep 13 2022, 2:17 AM
  • [InstCombine] Matrix multiplication negation optimisation
zjaffal updated this revision to Diff 459687.Sep 13 2022, 2:18 AM

Update tests

fhahn accepted this revision.Sep 13 2022, 2:23 AM

LGTM, thanks for the recent changes! If there are additional test cases needed those can be submitted as follow-up I think.

This revision is now accepted and ready to land.Sep 13 2022, 2:23 AM
This revision was automatically updated to reflect the committed changes.