This is an archive of the discontinued LLVM Phabricator instance.

[PatternMatch] Implement matching code for LibFunc
ClosedPublic

Authored by Quolyk on Jan 14 2018, 11:16 PM.

Diff Detail

Repository
rL LLVM

Event Timeline

Quolyk created this revision.Jan 14 2018, 11:16 PM

Up. I guess it still needs to be merged as it introduces pattern matching for LibFuncs that would be quite usefull.

Do you need to check whether the call is marked nobuiltin?

Quolyk updated this revision to Diff 155831.Jul 17 2018, 2:55 AM

Check if call isNoBuiltin

Is it possible to add some unit test for this?

Is it possible to add some unit test for this?

I have not seen that this header is explicitly tested, but it can be tested througn some libFunc pattern matches, as I hope to introduce some using this

Is it possible to add some unit test for this?

I have not seen that this header is explicitly tested, but it can be tested througn some libFunc pattern matches, as I hope to introduce some using this

unittests/IR/PatternMatch.cpp

efriedma added inline comments.Jul 17 2018, 10:48 AM
include/llvm/IR/PatternMatch.h
1648 ↗(On Diff #155831)

Please use TargetLibraryInfo::getLibFunc(ImmutableCallSite CS, LibFunc &F) when possible, so we can avoid scattering validity checks all over the code. (This formulation misses the call to isValidProtoForLibFunc.)

Quolyk updated this revision to Diff 185035.Feb 4 2019, 6:26 AM

Use TargetLibraryInfo::getLibFunc.

Herald added a project: Restricted Project. · View Herald TranscriptFeb 4 2019, 6:26 AM
Quolyk retitled this revision from Pattern matching code for LibFunc to [PatternMatch] Implement matching code for LibFunc.Feb 4 2019, 6:27 AM
Quolyk edited the summary of this revision. (Show Details)
Quolyk added a reviewer: lebedev.ri.
Quolyk added a comment.Feb 4 2019, 6:30 AM

Is it possible to add some unit test for this?

I have not seen that this header is explicitly tested, but it can be tested througn some libFunc pattern matches, as I hope to introduce some using this

unittests/IR/PatternMatch.cpp

I don't see that all patterns are tested in that file. Instead, patterns are tested implicitly in optimization transforms.

lebedev.ri requested changes to this revision.Jun 21 2019, 10:46 AM
This revision now requires changes to proceed.Jun 21 2019, 10:46 AM
Quolyk updated this revision to Diff 207042.Jun 28 2019, 5:11 AM

Add unit tests

Almost there i think..

unittests/IR/PatternMatch.cpp
637–645 ↗(On Diff #207042)

Nice.
I feel like the template logic in PatternMatch.h looks fragile, so this could use a new negative tests, too.

lebedev.ri requested changes to this revision.Jul 10 2019, 2:37 PM

(no new comments - still looks ok other than lack of at least one negative test - just marking as reviewed)

This revision now requires changes to proceed.Jul 10 2019, 2:37 PM
Quolyk updated this revision to Diff 209129.Jul 10 2019, 11:46 PM

Add negative tests

lebedev.ri added inline comments.Jul 10 2019, 11:56 PM
include/llvm/IR/PatternMatch.h
1802 ↗(On Diff #209129)

Does TLI.getLibFunc() check NoBuiltin internally?
I see that check got dropped.

Quolyk updated this revision to Diff 209133.Jul 11 2019, 12:19 AM

Use NoBuiltin

lebedev.ri accepted this revision.Jul 11 2019, 12:24 AM

Use NoBuiltin

Okay, should be fine i guess.
Should that be tested?

This revision is now accepted and ready to land.Jul 11 2019, 12:24 AM
Quolyk updated this revision to Diff 209460.Jul 12 2019, 4:33 AM

Add NoBuiltin tests.

Quolyk updated this revision to Diff 209461.Jul 12 2019, 4:35 AM

Minor update

This revision was automatically updated to reflect the committed changes.