This is an archive of the discontinued LLVM Phabricator instance.

[FPEnv] PatternMatcher support for checking FNEG ignoring signed zeros
ClosedPublic

Authored by cameron.mcinally on Oct 5 2018, 8:09 AM.

Details

Summary

@spatel

This is the first patch in a series to replace BinaryOperator::isFNeg(...) with more general pattern matcher calls.

We'll need a mechanism in the pattern matcher to match FNeg patterns that ignore signed zeros. I propose adding a new pattern matcher function, called m_ISZFNeg(...). This patch is one possible solution, but I'll leave it open for discussion. I'll also add that I have not worked on the pattern matcher before, so please review thoroughly.

There are a handful of tests that exercise the modified xform in test/Transforms/InstSimplify/fast-math.ll. So, I did not add new tests.

Diff Detail

Event Timeline

lib/Analysis/InstructionSimplify.cpp
4512

Quick note:

I was torn here about adding a second pointer temp for Op1's captured element. But, I believe that C++'s sequence points guarantee that the reuse of X is safe.

Small update to remove superfluous parentheses.

spatel added inline comments.Oct 7 2018, 7:38 AM
include/llvm/IR/PatternMatch.h
672

Let's use the existing LLVM vocabulary for "ignore signed zero":
"m_FNegNSZ"

lib/Analysis/InstructionSimplify.cpp
4510–4512

Yes, this works like you're hoping. We use this code pattern all over instsimplify/instcombine.

But we have a matcher API that will shorten this:

if (match(Op0, m_FNegNSZ(m_Specific(Op1) || 
    match(Op1, m_FNegNSZ(m_Specific(Op0))))
spatel added a comment.Oct 7 2018, 7:50 AM

This patch is not 'NFC'. I added a test that should show an improvement at rL343936.

include/llvm/IR/PatternMatch.h
672

Thanks, I didn't know that existed.

Do you want "NSZ" to be a suffix? I noticed that the others, like NSW, are prefixed: m_NSWAdd(...).

There are subtle differences between those, so I don't have a strong opinion either way.

spatel added inline comments.Oct 8 2018, 8:00 AM
include/llvm/IR/PatternMatch.h
672

Ah, I didn't notice that difference.

The suffixed version looks less like alphabet soup to me, corresponds to the order you'd see that string in IR, and corresponds to the "BinaryOperator::CreateFNegFMF()" format (bias: I added that).

So yes, my vote is m_FNegNSZ. Unless there's some reason to keep the existing names for the overflowing matchers, I'd change all of them as an NFC formatting commit for consistency.

Update 'ignore signed zero' acronym to "NSZ".

I decided against the suffix change we discussed, @spatel. It appears that the IRBuilder also uses the prefix format for "NSW" and similar. Any objections to this? There are subtle differences here, so I could be persuaded otherwise.

Also update the CHECKS for a newly added test (r343936).

Update 'ignore signed zero' acronym to "NSZ".

I decided against the suffix change we discussed, @spatel. It appears that the IRBuilder also uses the prefix format for "NSW" and similar. Any objections to this? There are subtle differences here, so I could be persuaded otherwise.

Also update the CHECKS for a newly added test (r343936).

NSW is a flag *on an instruction*. It's not a 'fneg operation of NSZ instruction'.
Suffix variants are cleaner IMO. Would be great to stick to them, and migrate the rest.

NSW is a flag *on an instruction*. It's not a 'fneg operation of NSZ instruction'.
Suffix variants are cleaner IMO. Would be great to stick to them, and migrate the rest.

I don't feel strongly either way, but will point out one inconsistency...

The only suffixes in IRBuilder are on CreateFxxxFMF(...). Those are instructions that copy over FastMath flags. That sounds like it falls into your "is a flag on an instruction" bucket and should have a prefix instead of a suffix. But again, there are subtle differences there. For example, CreateFxxxFMF(...) takes the flags as an input and CreateNSWyyy(...) has a fixed value for the flag.

Anyone else feel strongly about the prefix/suffix decision? I don't work in this part of LLVM enough to have a strong opinion.

Move "NSZ" to be a suffix.

cameron.mcinally marked 4 inline comments as done.Oct 9 2018, 1:35 PM
spatel accepted this revision.Oct 9 2018, 2:00 PM

LGTM

This revision is now accepted and ready to land.Oct 9 2018, 2:00 PM

Bah, I forgot the "Differential Revision:" tag. This was committed with:


r344084 | mcinally | 2018-10-09 16:48:00 -0500 (Tue, 09 Oct 2018) | 5 lines

[FPEnv] PatternMatcher support for checking FNEG ignoring signed zeros

https://reviews.llvm.org/D52934

Apologies for that...