This is an archive of the discontinued LLVM Phabricator instance.

AMDGPU: Fold fneg into bitcast of build_vector
ClosedPublic

Authored by arsenm on Jan 27 2023, 9:24 AM.

Details

Reviewers
foad
rampitec
Pierre-vh
Group Reviewers
Restricted Project
Summary

The math libraries have a lot of code that performs
manual sign bit operations by bitcasting doubles to int2
and doing bithacking on them. This is a bad canonical form
we should rewrite to use high level sign operations directly
on double. To avoid codegen regressions, we need to do a better
job moving fnegs to operate only on the high 32-bits.

This is only halfway to fixing the real case.

Diff Detail

Event Timeline

arsenm created this revision.Jan 27 2023, 9:24 AM
Herald added a project: Restricted Project. · View Herald TranscriptJan 27 2023, 9:24 AM
arsenm requested review of this revision.Jan 27 2023, 9:24 AM
Herald added a project: Restricted Project. · View Herald TranscriptJan 27 2023, 9:24 AM
Herald added a subscriber: wdng. · View Herald Transcript
Pierre-vh added inline comments.Jan 31 2023, 1:56 AM
llvm/lib/Target/AMDGPU/AMDGPUISelLowering.cpp
576–588
4161

Do a early return instead to reduce indentation?

4167–4181

Can you add a comment to show what this is doing? (A -> B style comment like we usually do for DAG combines)

arsenm updated this revision to Diff 494580.Feb 3 2023, 3:21 AM
arsenm marked an inline comment as done.

Address comments

llvm/lib/Target/AMDGPU/AMDGPUISelLowering.cpp
4161

That would just get re-indented in the next patch

Pierre-vh added inline comments.Feb 8 2023, 3:00 AM
llvm/lib/Target/AMDGPU/AMDGPUISelLowering.cpp
4173–4174

Wouldn't it make more sense to bitcast the fneg back to i32 to get 2xi32 -> f64? Would that change codegen?
2xf32 -> f64 looks wrong to me (even though it's correct in that context)

llvm/test/CodeGen/AMDGPU/fneg-modifier-casting.ll
517

There's a small regression here (1 extra instruction), is that addressed in a next patch?

917

ditto

arsenm added inline comments.Mar 15 2023, 3:28 PM
llvm/lib/Target/AMDGPU/AMDGPUISelLowering.cpp
4167–4181

That's already what's above?

4173–4174

That would just introduce new bitcasts, which would be equally foldable back. Minimal bitcasts is better. They get in the way enough as is

Rebase had a few regressions due to the revert of 11c3cead23783e65fb30e673d62771352078ff05

llvm/test/CodeGen/AMDGPU/fneg-modifier-casting.ll
517

Yes, there's additional source modifier usage after D142749

rampitec accepted this revision.Apr 10 2023, 10:22 AM
rampitec added inline comments.
llvm/lib/Target/AMDGPU/AMDGPUISelLowering.cpp
4176

This is really a hack, half of the f64 is not an f32. It's unfortunate we have to do it.

This revision is now accepted and ready to land.Apr 10 2023, 10:22 AM