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.
Details
Diff Detail
- Repository
- rG LLVM Github Monorepo
Event Timeline
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 |
- 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.
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. |
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)
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 :) ) |
llvm/utils/gn/secondary/llvm/lib/Target/AMDGPU/BUILD.gn | ||
---|---|---|
134 | Sorry, was not aware of this. Thanks for letting me know. |
I thought this already existed