This is an archive of the discontinued LLVM Phabricator instance.

[AMDGPU][GlobalISel] Fold G_FNEG above when users cannot fold mods
ClosedPublic

Authored by mbrkusanin on Oct 29 2021, 9:12 AM.

Details

Summary

If possible fold fneg into instruction above if users cannot fold mods and we
know it will decrease instruction count.
Follows same logic as SDAG combiner in choosing opportunities to combine.

Diff Detail

Event Timeline

mbrkusanin created this revision.Oct 29 2021, 9:12 AM
mbrkusanin requested review of this revision.Oct 29 2021, 9:12 AM

Following opcodes are not expected to appear after legalizer but I guess there is no harm keeping them.

G_FNEARBYINT:          // fails in legalizer (can be lowerd to frint)
G_INTRINSIC_ROUND:     // legalized into other instructions
G_INTRINSIC_ROUNDEVEN: // legalized into rint + some other instructions
G_FSIN:                // lowered into amdgcn_sin

We should also apply these pre-legalization like in the DAG

llvm/lib/CodeGen/GlobalISel/CombinerHelper.cpp
161–169

I thought this already existed

mbrkusanin updated this revision to Diff 386214.EditedNov 10 2021, 10:07 AM
  • Added AMDGPUCombinerHelper, a common helper for both amdgpu-prelegalizer-combiner and amdgpu-postlegalizer-combiner pass. This way same combine can be used in both passes.

Since this is a AMD specific combine it made no sense to put in generic CombinerHelper.

mbrkusanin added inline comments.Nov 10 2021, 10:09 AM
llvm/lib/CodeGen/GlobalISel/CombinerHelper.cpp
161–169

There is LegalizerHelper::changeOpcode but would instantiating LegalizerHelper just for this be practical?

Observer is a protected member so it can't be used directly which is why I needed this.

arsenm added inline comments.Nov 10 2021, 2:40 PM
llvm/lib/Target/AMDGPU/AMDGPUCombinerHelper.h
21

Would make more sense to me to inherit from CombinerHelper

  • AMDGPUCombinerHelper now inherits CombinerHelper class.
  • Now using AMDGPUCombinerHelper as one of helpers in prelegalizer and postlegalzer combiner passes instead of both AMDGPUCombinerHelper and CombinerHelper (does not affect tablegen generated code that much)
arsenm accepted this revision.Nov 15 2021, 6:08 PM
This revision is now accepted and ready to land.Nov 15 2021, 6:08 PM
thakis added a subscriber: thakis.Nov 17 2021, 10:35 AM
thakis added inline comments.
llvm/utils/gn/secondary/llvm/lib/Target/AMDGPU/BUILD.gn
134

FYI, no need to update the files in llvm/utils/gn. They're auto-synced from the cmake files in most cases.

(The autosyncer would've added this line with the required trailing comma too :) )

mbrkusanin added inline comments.Nov 18 2021, 1:03 AM
llvm/utils/gn/secondary/llvm/lib/Target/AMDGPU/BUILD.gn
134

Sorry, was not aware of this. Thanks for letting me know.