[Relay] Feature Detection #3238
Conversation
|
@wweic @ZihengJiang @vinx13 @srkreddy1238 @nhynes can you guys give it a round of review? it is ready. |
|
@tqchen @jroesch @slyubomirsky can you guys give it a round of review? |
| namespace relay { | ||
|
|
||
| /*! \brief Different kinds of relay feature a program might use. */ | ||
| enum Feature : int { |
There was a problem hiding this comment.
My only concern is that whether we need feature types for each kind of expr or only define a few high-level features like fGraph, fPatternMatch.
I would suggest putting these design to the RFC so that we can further discuss
There was a problem hiding this comment.
I think we should at least have one of each kind, as a pass might simply fail due to not pattern matching against it in the visitor.
There was a problem hiding this comment.
A pass failing due to missing visitor cases seems like the sort of thing that tests should catch. I agree with @vinx13 that it's probably better for these flags to correspond to high-level features
There was a problem hiding this comment.
The idea is that there might be two different characteristic for a pass: not matching against case x because it is unsupported, and not matching against case x because someone forgot to. We can use the f flag to detect such case, then use it to generate/pull all relay sample program satisfying the feature, and finally run the test automatically, instead of writing basically the same test over and over.
|
At a high level, the implementation in this PR seems very reasonably written. I think it would be good to have unit tests, many tiny programs that each exhibit the specific features. Checking for graphs, recursion, etc. are good as tricky cases (also, should global var recursion be checked too? Many prelude functions are recursive). I think there should be discussion about the concern @vinx13 raised in his comment, as to whether there should be more high-level features and what those features should be. (I don't think you should get rid of per-AST node checks, as they can be useful for some passes, but it's worth asking what other features could be good to have.) |
|
@slyubomirsky all feature is tested (graph one is tested in test_anf/test_gnf). |
|
I realize that the features appear, but I think it would be good to have unit tests for each feature specifically so we can get a more accurate idea of what is wrong if future changes break something |
|
@yzhliu @tqchen @ZihengJiang @vinx13 @junrushao1994 can you guys give another round of review? I need this as it make testing stuff easier. |
* init init lint rename ci fix add add some doc save add some test add some test lint lint lint * fix build
* init init lint rename ci fix add add some doc save add some test add some test lint lint lint * fix build
this is the implementation of #3236.