Page MenuHomePhabricator

[clang-tidy] new opencl recursion not supported check
Needs RevisionPublic

Authored by ffrankies on Jan 5 2020, 5:39 PM.

Details

Summary

This lint check is part of the FLOCL (FPGA Linters for OpenCL) project out of the Synergy Lab at Virginia Tech.

FLOCL is a set of lint checks aimed at FPGA developers who code in OpenCL.

The opencl recursion not supported check is placed in a new "opencl" module.

The check finds and flags recursive function calls, which are not supported by the OpenCL standard.

As per the official OpenCL restrictions list.

Diff Detail

Event Timeline

ffrankies created this revision.Jan 5 2020, 5:39 PM

I think will be good idea to separate module code in own review or refer to previous one of previous reviews as dependency.

clang-tidy/opencl/RecursionNotSupportedCheck.cpp
28

const auto *

29

const auto *

53

const auto &, because Loc is iterator.

76

{}

78

const auto &, because Caller is iterator.

108

Are two steps and temporary variable are really necessary?

docs/ReleaseNotes.rst
131

Please synchronize with first statement in documentation.

docs/clang-tidy/checks/opencl-recursion-not-supported.rst
38

Please highlight 100 with single back-quotes.

mgehre removed a subscriber: mgehre.Jan 5 2020, 10:59 PM
lebedev.ri requested changes to this revision.Jan 5 2020, 11:17 PM
lebedev.ri added a subscriber: lebedev.ri.

I was just going to look into this kind of check.
This can be implemented without recursion in the check itself.
Let me write it and post my variant.

This revision now requires changes to proceed.Jan 5 2020, 11:17 PM

I was just going to look into this kind of check.
This can be implemented without recursion in the check itself.
Let me write it and post my variant.

D72362

The other recursion check seems more sophisticated. What is your take on it? Would you consider it better as well, what is your opinion on it?

The other recursion check seems more sophisticated. What is your take on it? Would you consider it better as well, what is your opinion on it?

I agree, I would go with the other check if I had to choose between the two. Since @lebedev.ri's version uses an existing CallGraph structure instead of re-creating it (like we did), I'd guess it is more performant than our version. Plus it doesn't have a user-defined recursion depth limit, which is one less thing for the user to worry about.

D72362 has landed.
I'd advise to see if any of the testcases here aren't being reported by that check, and filing a bugs for them.
Other than that, abandon?