Page MenuHomePhabricator

[PatternMatch] Add new FP matchers. NFC.
ClosedPublic

Authored by foad on Oct 8 2020, 6:09 AM.

Details

Summary

This adds matchers m_NonNaN, m_NonInf, m_Finite and m_NonZeroFP as well
as generic support for binding the matched value to an APFloat.

I tried to follow the existing convention of using an FP suffix for
predicates like zero and non-zero, which could be confused with the
integer versions, but not for predicates which are clearly already
FP-specific.

Diff Detail

Event Timeline

foad created this revision.Oct 8 2020, 6:09 AM
Herald added a project: Restricted Project. · View Herald TranscriptOct 8 2020, 6:09 AM
foad requested review of this revision.Oct 8 2020, 6:09 AM
RKSimon added inline comments.Oct 8 2020, 6:50 AM
llvm/lib/Analysis/ValueTracking.cpp
3665 ↗(On Diff #296942)

Would it be better to simplify this entire loop to match(V, m_NonNaN()) ?

foad updated this revision to Diff 296966.Oct 8 2020, 7:19 AM

Use matchers to handle fixed width vectors.

RKSimon added inline comments.Oct 9 2020, 3:37 AM
llvm/lib/Analysis/InstructionSimplify.cpp
4039 ↗(On Diff #296966)

I'd expect this now to work for non-uniform vectors that contain different non-zero float values (and also undefs) - can we add test coverage for this please?

llvm/lib/Analysis/ValueTracking.cpp
5297 ↗(On Diff #296966)

Ideally this function needs extending to support non-uniform vectors as well.

5310 ↗(On Diff #296966)

m_APFloat didn't match constant vectors containing undefs but m_Finite will? Again, we need test coverage for this.

foad updated this revision to Diff 298599.Oct 16 2020, 5:02 AM

Test simplifySelectWithFCmp on non-uniform vectors.

foad marked an inline comment as done.Oct 16 2020, 5:05 AM
foad added inline comments.
llvm/lib/Analysis/ValueTracking.cpp
5297 ↗(On Diff #296966)

Sure, but I'd prefer to do that in a separate patch. Currently it relies on checks like *FC1 < *FC2 below that won't work for vectors, and I'm not sure how to write that in a vector-friendly way.

5310 ↗(On Diff #296966)

Since this whole function does support vectors yet, there's NFC here and nothing to test.

RKSimon added inline comments.Oct 16 2020, 5:33 AM
llvm/lib/Analysis/ValueTracking.cpp
5310 ↗(On Diff #296966)

OK - in which case a TODO comment plus an assert that !getType->isVectorTy() at the top of the function would be fine.

foad updated this revision to Diff 298608.Oct 16 2020, 5:56 AM

Add a vector test case for matchFastFloatClamp.

foad added inline comments.Oct 16 2020, 5:59 AM
llvm/lib/Analysis/ValueTracking.cpp
5310 ↗(On Diff #296966)

I was mistaken. This function did already handle splat vectors with no undefs, because of the use of m_APFloat matchers. I've added an explicit test case for this. My patch does not change this behaviour.

@spatel @lebedev.ri any additional comments?

lebedev.ri added inline comments.Oct 22 2020, 3:07 AM
llvm/include/llvm/IR/PatternMatch.h
338–340

This should probably mention whether or not undef vector constants are allowed.

353

Note that this is implicitly /*AllowUndefs=*/false

589

I think i'd like to see some dedicated tests with undefs in llvm/unittest/IR/PatternMatch.cpp.

foad updated this revision to Diff 299955.Oct 22 2020, 6:58 AM

Add comments.

foad marked an inline comment as done.Oct 22 2020, 7:01 AM
foad added inline comments.
llvm/include/llvm/IR/PatternMatch.h
353

Right, so there is a pre-existing problem with matchers like this one, which claim to allow undef, but in fact the second (binding) form of the matcher does not:

/// Match an integer or vector of negative values.
/// For vectors, this includes constants with undefined elements.
inline cst_pred_ty<is_negative> m_Negative() {
  return cst_pred_ty<is_negative>();
}
inline api_pred_ty<is_negative> m_Negative(const APInt *&V) {
  return V;
}
foad updated this revision to Diff 299965.Oct 22 2020, 7:24 AM

Add unit tests.

lebedev.ri added inline comments.Oct 22 2020, 7:29 AM
llvm/include/llvm/IR/PatternMatch.h
589

So does it or does it not allow undefs?

RKSimon added inline comments.Oct 22 2020, 7:39 AM
llvm/include/llvm/IR/PatternMatch.h
589

AFAICT cstfp_pred_ty supports vectors with undef elements but apf_pred_ty doesnot

foad added inline comments.Oct 22 2020, 7:40 AM
llvm/include/llvm/IR/PatternMatch.h
589

It allows undefs.

I'm lost.
If their code related to handling splats is identical (neither of them pass anything into getSplatValue()), why do they end up with different undef behaviour?

foad added a comment.Oct 22 2020, 8:18 AM

I'm lost.
If their code related to handling splats is identical (neither of them pass anything into getSplatValue()), why do they end up with different undef behaviour?

cstfp_pred_ty has its own handling of non-splat vector constants. apf_pred_ty does not. I realise this is not great! It was copied directly from the integer equivalents, which have the same inconsistency.

lebedev.ri added inline comments.Oct 22 2020, 8:33 AM
llvm/include/llvm/IR/PatternMatch.h
616–620

So if this is using apf_pred_ty, then this doesn't support undef constants then?

foad added inline comments.Oct 22 2020, 8:41 AM
llvm/include/llvm/IR/PatternMatch.h
616–620

Correct. m_Finite() does, m_Finite(const APFloat *&) does not. This is the pre-existing problem I mentioned which has now been copied across from the integer matchers to the fp ones.

Anyways i think this looks fine to me now. Thanks.

llvm/include/llvm/IR/PatternMatch.h
616

But it says it does.

foad updated this revision to Diff 300005.Oct 22 2020, 9:03 AM

Change apf_pred_ty to allow undefs and test that this works for m_Finite(const APFloat *&).

foad added inline comments.Oct 22 2020, 9:05 AM
llvm/include/llvm/IR/PatternMatch.h
616

Yes, that's the pre-existing bug! Anyway I have now fixed this for the new float matchers, but I have not touched the existing integer matchers.

lebedev.ri added inline comments.Oct 22 2020, 9:07 AM
llvm/include/llvm/IR/PatternMatch.h
616

Okay, you lost me there.
How it's a "pre-existing bug", if you add this matcher here?

foad added inline comments.Oct 22 2020, 9:14 AM
llvm/include/llvm/IR/PatternMatch.h
616

I should have said: a pre-existing bug in the integer matchers, which I copied across to the new fp matchers.

lebedev.ri accepted this revision.Oct 22 2020, 10:24 AM

I would strongly recommend to split this into two separate commits - adding the matchers, and using them.
Because while the matchers themselves LG, i'm not at all confident with users.
When doing latter, please ensure that each separate change has appropriate undef test coverage, and ideally check that alive2 is happy about those transforms.

This revision is now accepted and ready to land.Oct 22 2020, 10:24 AM
foad added a comment.Oct 22 2020, 11:24 AM

I would strongly recommend to split this into two separate commits - adding the matchers, and using them.

Can do. But it feels like every time I try to separate things like that, some reviewer asks me to put it all into one patch again :( https://reviews.llvm.org/D87034#2254864

This revision was landed with ongoing or failed builds.Oct 22 2020, 11:43 AM
This revision was automatically updated to reflect the committed changes.
foad added a comment.Oct 23 2020, 2:02 AM

I would strongly recommend to split this into two separate commits - adding the matchers, and using them.

OK, I split the "using them" part out into D90015.