You signed in with another tab or window. Reload to refresh your session.You signed out in another tab or window. Reload to refresh your session.You switched accounts on another tab or window. Reload to refresh your session.Dismiss alert
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This PR combines the ast_plan and linearizer classes into the new expression_parser class. Previously the linearizer would parse the expression into some host-side buffers, and the ast_plan simply wrapped a linearizer instance and transferred its contents into an owned device buffer so that expressions could be evaluated in device code. This required either adding lots of accessor methods to otherwise internal components of the linearizer or adding friend relationships. Since we have no use for parsing expressions without making them evaluable on device, this PR combines the two classes to simplify that process.
There's one additional minor change which is the move of the single_dispatch_binary_operator functor from operators.hpp to transform.cuh. It's only being used in one place, so this change reduces the need for includes and makes the code more readable.
Nope this change is performance-neutral. It simply converts what was previously an implicit relationship (the linearizer and ast_plan had to be created together, and the sole purpose of the ast_plan was to perform a H2D transfer of the linearizer's internal data) into an explicit one that also reduces some boilerplate.
CC @jlowe you mentioned that you're pulling in the detail::node struct for the JNI. I looked at java/src/main/native/src/CompiledExpression.cpp and it looks like it's being implicitly included via public headers, so I don't think this rename will cause any problems for your implementation, but wanted to ping you just in case there's another file referencing these internals that I missed.
The reason will be displayed to describe this comment to others. Learn more.
Still need to convert to east const convention in some places. I didn't point out all those, so please read and check the entire code. Otherwise, it is good.
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
vyasr
added
3 - Ready for Review
This PR combines the
ast_planandlinearizerclasses into the newexpression_parserclass. Previously thelinearizerwould parse the expression into some host-side buffers, and theast_plansimply wrapped alinearizerinstance and transferred its contents into an owned device buffer so that expressions could be evaluated in device code. This required either adding lots of accessor methods to otherwise internal components of thelinearizeror adding friend relationships. Since we have no use for parsing expressions without making them evaluable on device, this PR combines the two classes to simplify that process.There's one additional minor change which is the move of the
single_dispatch_binary_operatorfunctor fromoperators.hpptotransform.cuh. It's only being used in one place, so this change reduces the need for includes and makes the code more readable.Contributes to #8145.