This is an archive of the discontinued LLVM Phabricator instance.

Fix m_[Ord|Unord][FMin|FMax] matchers to correctly match ordering.
ClosedPublic

Authored by a.elovikov on May 15 2017, 4:11 AM.

Details

Summary

Previously, the matching was done incorrectly for the case where
operands for FCmpInst and SelectInst were in opposite order.

Event Timeline

a.elovikov created this revision.May 15 2017, 4:11 AM
spatel edited edge metadata.Jun 12 2017, 9:06 AM

Sorry for the delay. Can this bug be exposed with an IR transform or a codegen test?

Sorry for the delay. Can this bug be exposed with an IR transform or a codegen test?

I am not aware of such cases. I've faced that when working on D33186 but later decided to work with fast math only there. But I believe that this is still useful to fix. At least someone else won't be surprised by it.

spatel accepted this revision.Jun 12 2017, 9:39 AM

Sorry for the delay. Can this bug be exposed with an IR transform or a codegen test?

I am not aware of such cases. I've faced that when working on D33186 but later decided to work with fast math only there. But I believe that this is still useful to fix. At least someone else won't be surprised by it.

Agreed - we need to fix the bug. LGTM. Please see inline comments too for some cleanup.

llvm/include/llvm/IR/PatternMatch.h
1141

max(L, R) --> min(L, R)

1157

m_UnordFMin(L, R) --> m_UnordFMax(L, R)

1224–1225

max(L, R) --> min(L, R)

1224–1225

This should be moved up with the related fmin/fmax templates.

This revision is now accepted and ready to land.Jun 12 2017, 9:39 AM
a.elovikov updated this revision to Diff 102296.EditedJun 13 2017, 12:45 AM
  • Fix typos in comments/move m_UnordFMin function as asked.
a.elovikov marked 4 inline comments as done.Jun 13 2017, 12:47 AM

Thanks! Do you need someone to commit this for you?

@spatel Yes he does. He sent me a mail asking for me to commit it for him. Do you want to do it or do you want me to?

@spatel Yes he does. He sent me a mail asking for me to commit it for him. Do you want to do it or do you want me to?

Please go ahead if you have that queued up.

This revision was automatically updated to reflect the committed changes.