This is an archive of the discontinued LLVM Phabricator instance.

AMDGPU: Try to select fmul by power of 2 to ldexp
ClosedPublic

Authored by arsenm on Aug 4 2023, 8:39 AM.

Details

Reviewers
foad
Pierre-vh
rampitec
Group Reviewers
Restricted Project
Summary

For the f64 case, this gives us a cheaper to materialize 32-bit
constant. It's less obviously a win for f32 and f16. It forces us to
use a VOP3 encoding so it's a neutral code size change.

GlobalISel cases don't work because of the constant-is-copy-to-vgpr
problem.

Diff Detail

Event Timeline

arsenm created this revision.Aug 4 2023, 8:39 AM
Herald added a project: Restricted Project. · View Herald TranscriptAug 4 2023, 8:39 AM
arsenm requested review of this revision.Aug 4 2023, 8:39 AM
Herald added a project: Restricted Project. · View Herald TranscriptAug 4 2023, 8:39 AM
Herald added a subscriber: wdng. · View Herald Transcript
foad added inline comments.Aug 4 2023, 8:49 AM
llvm/lib/Target/AMDGPU/SIInstructions.td
3334

Should be >= -1. As is you are treating +/-0.25 as an inline.

llvm/test/CodeGen/AMDGPU/GlobalISel/inst-select-fmul.mir
277

Need some tests with negated powers of two. I don't see how the patch handles these.

arsenm added inline comments.Aug 4 2023, 8:57 AM
llvm/test/CodeGen/AMDGPU/GlobalISel/inst-select-fmul.mir
277

this is just to demonstrate the patterns do actually import, the actual testing should be the IR test (I'm just working around the failure to import)

arsenm updated this revision to Diff 547335.Aug 4 2023, 1:46 PM
arsenm marked an inline comment as done.
foad requested changes to this revision.Aug 7 2023, 1:51 AM
foad added inline comments.
llvm/lib/Target/AMDGPU/AMDGPUInstructionSelector.cpp
5107

This makes no sense at all. The sign of APF has nothing to do with the sign of ExpVal. If ExpVal is negative that means that the magnitude of APF is < 1, irrespective of its sign.

This revision now requires changes to proceed.Aug 7 2023, 1:51 AM
arsenm updated this revision to Diff 547967.Aug 7 2023, 3:03 PM

Revert nonsense

foad added a comment.Aug 10 2023, 5:34 AM

Any plans to do something for negated powers of two?

llvm/lib/Target/AMDGPU/SIInstructions.td
3330
3335
3342

Don't need parens.

3349
3353

Too many parens.

foad added a comment.Aug 10 2023, 5:34 AM

Is there some way I can remove my "requested changes" flag, without accepting it yet?

Is there some way I can remove my "requested changes" flag, without accepting it yet?

That happens whenever the diff is updated, I don't think you can do it yourself

arsenm updated this revision to Diff 549210.Aug 10 2023, 5:26 PM
arsenm marked 3 inline comments as done.
foad added a comment.Aug 11 2023, 1:25 AM

Is there some way I can remove my "requested changes" flag, without accepting it yet?

That happens whenever the diff is updated, I don't think you can do it yourself

I see. I was confused by the red X next to my name at the top of the web page, which apparently just means I requested changes to a prior diff.

I see. I was confused by the red X next to my name at the top of the web page, which apparently just means I requested changes to a prior diff.

Previously I have only found a way to w/a it by accepting a new diff. Which makes sense.

foad accepted this revision.Aug 11 2023, 1:30 AM
This revision is now accepted and ready to land.Aug 11 2023, 1:30 AM