This is an archive of the discontinued LLVM Phabricator instance.

[AMDGPU] Make some VOP3 insts commutable
ClosedPublic

Authored by Joe_Nash on Apr 28 2021, 10:58 AM.

Details

Summary

Note, only src0 and src1 will be commuted if the isCommutable flag
is set. This patch does not change that, it just makes it possible
to commute src0 and src1 of some U/I/B vop3 instructions.

This patch revises d35d8da7d6ac6c08578ec0569b072292631691e0.
It contains the commute opportunities excluding float insts

Diff Detail

Event Timeline

Joe_Nash created this revision.Apr 28 2021, 10:58 AM
Joe_Nash requested review of this revision.Apr 28 2021, 10:58 AM
Herald added a project: Restricted Project. · View Herald TranscriptApr 28 2021, 10:58 AM
rampitec accepted this revision.Apr 28 2021, 11:04 AM
This revision is now accepted and ready to land.Apr 28 2021, 11:04 AM
This revision was landed with ongoing or failed builds.Apr 28 2021, 11:10 AM
This revision was automatically updated to reflect the committed changes.
foad added a comment.Apr 29 2021, 2:31 AM

This patch revises d35d8da7d6ac6c08578ec0569b072292631691e0.
It contains the commute opportunities excluding float insts

Maybe mention D99376 here rather than just the commit hash?

Why are you excluding float insts now? Is it related to the test failures that caused you to revert D99376, or is it a different problem?

This patch revises d35d8da7d6ac6c08578ec0569b072292631691e0.
It contains the commute opportunities excluding float insts

Maybe mention D99376 here rather than just the commit hash?

Why are you excluding float insts now? Is it related to the test failures that caused you to revert D99376, or is it a different problem?

That is what you suggested last time on https://reviews.llvm.org/D99376

foad added inline comments.Mar 30 2021, 2:08 AM
llvm/lib/Target/AMDGPU/VOP3Instructions.td

371
You have to be super careful with fp min/max/med because the NaN handling is not commutative. You could commute them with suitable "nnan" or other IEEE-related flags, but it's probably not worth it. So I would suggest dropping them.

632
Likewise, drop f16 min/max/med.

foad added a comment.Apr 29 2021, 9:44 AM

That is what you suggested last time on https://reviews.llvm.org/D99376

OK :-)

So what caused the test failures that prompted the revert of D99376? Did you just need to update some more lit tests?

That is what you suggested last time on https://reviews.llvm.org/D99376

OK :-)

So what caused the test failures that prompted the revert of D99376? Did you just need to update some more lit tests?

In between when I put up the review and landed the patch, someone introduced new tests in the AMDGPU backend that differed with the commute changes. So I did a prompt rebase this time when I submitted this patch.