Page MenuHomePhabricator

[InstCombine] form copysign from select of FP constants (PR44153)
ClosedPublic

Authored by spatel on Jan 13 2020, 1:38 PM.

Details

Summary

This should be the last step needed to solve the problem in the description of PR44153:
https://bugs.llvm.org/show_bug.cgi?id=44153

If we're casting an FP value to int, testing its signbit, and then choosing between a value and its negated value, that's a complicated way of saying "copysign":

(bitcast X) <  0 ? -TC :  TC --> copysign(TC,  X)

Diff Detail

Event Timeline

spatel created this revision.Jan 13 2020, 1:38 PM
Herald added a project: Restricted Project. · View Herald TranscriptJan 13 2020, 1:39 PM
spatel edited reviewers, added: nick; removed: kaniandr.Jan 13 2020, 1:39 PM
nick added a comment.Jan 13 2020, 2:23 PM

I cannot be a reviewer, just added some dummies questions. And, @spatel, thanks for working on this!

llvm/lib/Transforms/InstCombine/InstCombineSelect.cpp
2331

I would never expect TC == FC to happen here. Are branches folded later than this transform?

2338

Is the one use check necessary? I would imagine that this transform will unblock other one use transforms.

spatel marked 2 inline comments as done.Jan 14 2020, 4:56 AM
spatel added inline comments.
llvm/lib/Transforms/InstCombine/InstCombineSelect.cpp
2331

We have to look at this 'select' independently of any other instruction because of the way instcombine processes. But I think you're correct that we can assume TC != FC because that should be simplified before we reach here. I will change that to an assert.

2338

It's not strictly necessary, but the transform becomes less clearly canonical in the case of >1 use because we can end up with more instructions than we started with (and I'm not sure what that will do in the backend/codegen). For those reasons, I think we should defer that change to its own minimal follow-on patch if we want to try it.

xbolva00 accepted this revision.Jan 16 2020, 11:31 AM

Looks ok

This revision is now accepted and ready to land.Jan 16 2020, 11:31 AM
xbolva00 added inline comments.Jan 16 2020, 11:32 AM
llvm/lib/Transforms/InstCombine/InstCombineSelect.cpp
2331

+1 for assert

spatel planned changes to this revision.Jan 16 2020, 12:25 PM
spatel marked an inline comment as done.
spatel added inline comments.
llvm/lib/Transforms/InstCombine/InstCombineSelect.cpp
2331

After I wrote that comment, I thought more about the vector case (undef lanes are always a pain!), and we can't assert yet because we're missing a simplify:
rG26d2ace9e230

And that's the motivation for:
D72784

So let me mark this patch as on hold for the moment and try to make progress on the underlying analysis.

nikic added a subscriber: nikic.Jan 17 2020, 2:17 PM
nikic added inline comments.
llvm/lib/Transforms/InstCombine/InstCombineSelect.cpp
2331

FWIW the m_APInt and m_APFloat matchers don't allow undefs. Only m_Zero and friends allow undefs.

spatel marked 2 inline comments as done.Jan 20 2020, 7:04 AM
spatel added inline comments.
llvm/lib/Transforms/InstCombine/InstCombineSelect.cpp
2331

This is correct. We'll have an option after D72975, but there really wasn't a reason to hold this up currently. I can change to an assert and push this.

spatel marked an inline comment as done.Jan 20 2020, 7:12 AM
spatel updated this revision to Diff 239124.Jan 20 2020, 7:24 AM

Patch updated:
Changed check for equal constants to assert (I changed one of the tests to vectors with undef, so we can enhance that as a follow-up).

This revision is now accepted and ready to land.Jan 20 2020, 7:24 AM
This revision was automatically updated to reflect the committed changes.