This is an archive of the discontinued LLVM Phabricator instance.

AMDGPU/GlobalISel: Add floating point med3 combine
ClosedPublic

Authored by Petar.Avramovic on Oct 23 2020, 8:42 AM.

Details

Summary

Add floating point version of med3 combine.
Source is fminnum(fmaxnum(Val, K0), K1) or fmaxnum(fminnum(Val, K1), K0)
where K0 and K1 are constants and K0 <= K1.

Diff Detail

Event Timeline

Herald added a project: Restricted Project. · View Herald TranscriptOct 23 2020, 8:42 AM
Petar.Avramovic requested review of this revision.Oct 23 2020, 8:42 AM

Missing tests for f16 and v2f16 cases

llvm/lib/Target/AMDGPU/AMDGPUPostLegalizerCombiner.cpp
174–175 ↗(On Diff #300311)

Might as well handle this now

181–182 ↗(On Diff #300311)

You can directly pass the APFloat to is the overload of isInlineConstant

llvm/lib/Target/AMDGPU/AMDGPUPostLegalizerCombiner.cpp
174–175 ↗(On Diff #300311)

I am not really sure I know correct way to do it. Also this combine heavily depends on legalizer and primarily completing remaining of SNaN checks in isKnownNeverSNaN.

foad added inline comments.Oct 27 2020, 3:44 AM
llvm/lib/Target/AMDGPU/AMDGPUPostLegalizerCombiner.cpp
172 ↗(On Diff #300311)

This will probably warn that Info is unused in a Release build.

Petar.Avramovic retitled this revision from AMDGPU/GlobalISel: Add floating point med3 combine for IEEE=false to AMDGPU/GlobalISel: Add floating point med3 combine.
Petar.Avramovic edited the summary of this revision. (Show Details)

Rebase/rework.

arsenm added inline comments.Jun 30 2021, 6:03 PM
llvm/lib/Target/AMDGPU/AMDGPURegBankCombiner.cpp
198

Like in the other patches, why do you need to re-query getConstantFPVRegVal? Why doesn't the constant matcher just directly return the constant?

Use matcher that returns both VReg and APFloat from G_FCONSTANT.

Rebase and ping.

arsenm added inline comments.Dec 1 2021, 3:55 PM
llvm/lib/Target/AMDGPU/AMDGPURegBankCombiner.cpp
194

I don't see why to make these optional if they won't be set or used if the matcher return false

211

This should be cached in the helper?

Cache SIInstrInfo in the helper.

Petar.Avramovic added inline comments.Dec 2 2021, 8:52 AM
llvm/lib/Target/AMDGPU/AMDGPURegBankCombiner.cpp
194

Util helpers and matchers use Optional.

arsenm accepted this revision.Dec 2 2021, 9:10 AM
This revision is now accepted and ready to land.Dec 2 2021, 9:10 AM
This revision was landed with ongoing or failed builds.Dec 3 2021, 4:02 AM
This revision was automatically updated to reflect the committed changes.