Page MenuHomePhabricator

[PatternMatch] allow intrinsic form of min/max with existing matchers
ClosedPublic

Authored by spatel on Aug 4 2020, 11:12 AM.

Details

Summary

I skimmed the existing users of these matchers and don't see any problems (eg, the caller assumes the matched value was a select instruction without checking).

So I think we can generalize the matching to allow the new intrinsics or the cmp+select idioms.

I did not find any unit tests for the matchers, so added some basics there. The instsimplify tests are adapted from existing tests for the cmp+select pattern and cover the folds in simplifyICmpWithMinMax().

Diff Detail

Event Timeline

spatel created this revision.Aug 4 2020, 11:12 AM
Herald added a project: Restricted Project. · View Herald TranscriptAug 4 2020, 11:12 AM
Herald added a subscriber: mcrosier. · View Herald Transcript
spatel requested review of this revision.Aug 4 2020, 11:12 AM

Maybe make it clear at the start of InstCombinerImpl::foldSelectOpOp where we use the matchers that we can safely use the matchers for the select(icmp) pattern OR the intrinsics?

spatel added a comment.Aug 4 2020, 2:18 PM

Maybe make it clear at the start of InstCombinerImpl::foldSelectOpOp where we use the matchers that we can safely use the matchers for the select(icmp) pattern OR the intrinsics?

In that code, we already know that the value that we are matching is a select, so there's no chance of matching an intrinsic there:

Instruction *InstCombinerImpl::foldSelectOpOp(SelectInst &SI, Instruction *TI,
                                              Instruction *FI) {
  if ((match(&SI, m_SMin(m_Value(), m_Value())) ||

We should eventually remove that code. Ie, once we canonicalize to the intrinsics, we will get to remove min/max crippling checks like that one because we won't have the cmp+sel patterns that correspond to min/max.

RKSimon accepted this revision.Aug 5 2020, 9:57 AM

Thanks for checking! LGTM

This revision is now accepted and ready to land.Aug 5 2020, 9:57 AM