This is an archive of the discontinued LLVM Phabricator instance.

AMDGPU: Pattern match fract instructions in AMDGPUCodeGenPrepare
ClosedPublic

Authored by arsenm on May 5 2023, 5:31 PM.

Details

Reviewers
foad
b-sumner
Pierre-vh
Group Reviewers
Restricted Project
Summary

This will allow eliminating the intrinsic uses in the device
libraries, which will remove a subtarget dependency on the f16
version of the intrinsic.

We previously had some wrong patterns for this under unsafe math
which I've removed.

Do it in IR partially to take advantage of the much better isKnownNeverNaN
handling, and partially out of laziness to avoid repeating this in the DAG
and GlobalISel path. Plus I think this should be done much earlier. Ideally
this would be in InstCombine, but you can't introduce target intrinsics
from a generic instruction rooted pattern.

Diff Detail

Event Timeline

arsenm created this revision.May 5 2023, 5:31 PM
Herald added a project: Restricted Project. · View Herald TranscriptMay 5 2023, 5:31 PM
arsenm requested review of this revision.May 5 2023, 5:31 PM
Herald added a project: Restricted Project. · View Herald TranscriptMay 5 2023, 5:31 PM
Herald added a subscriber: wdng. · View Herald Transcript

I just have some minor nits. I can't comment on the correctness of the logic so I will leave it up to someone with more experience w.r.t FP ops to approve

We previously had some wrong patterns for this under unsafe math which I've removed.

Would it be possible to split that in a separate patch? Without that it's a bit difficult to tell if a test changed because of the combine, or the pattern.

llvm/lib/Target/AMDGPU/AMDGPUCodeGenPrepare.cpp
228

nit: isn't IRBuilderBase better?

1423

(optional) small nit: maybe use !match and an early return to reduce nesting? Could also do !Fract and early return.

1680

nit: use unsigned as it's a size type?

arsenm updated this revision to Diff 520714.May 9 2023, 8:38 AM
arsenm marked 2 inline comments as done.
arsenm added inline comments.
llvm/lib/Target/AMDGPU/AMDGPUCodeGenPrepare.cpp
228

Don't see the point of making the type name longer here, it's not going to use any other IRBuilder

Pierre-vh accepted this revision as: Pierre-vh.May 17 2023, 2:46 AM

I don't have any other comments and I don't see any obvious regressions so LGTM. Though, it might be a good idea to wait for another reviewer with more experience with FP ops to comment before landing.

This revision is now accepted and ready to land.May 17 2023, 2:46 AM