Add a parser for the experiment annotation. #156
Conversation
This fixes some issues where my IDE does not recognize the header files.
There was a problem hiding this comment.
The main suggestion is to defer semantic-related analyses to the lowering stage, keeping the structure of the AST nodes as close as possible to the language grammar.
The logic currently implemented in the ExperimentAnnotation class could eventually be moved to the lib/Codegen/Lowering components. More specifically, it might fit well in the ModelLowerer class, since the semantics of the annotation are closely tied to the model entity. In that context, you could traverse the annotation tree and attach the relevant information to the bmodelica.model operation. To support this, your ExperimentAnnotation class can naturally evolve into an MLIR attribute.
I've also left some other minor comments.
|
|
||
| include(GNUInstallDirs) | ||
| include(FetchContent) | ||
| include_directories(${CMAKE_SOURCE_DIR}/include) |
There was a problem hiding this comment.
This is already handled by the MARCO_INCLUDE_DIR macro within the same file.
| return result; | ||
| } | ||
|
|
||
| bool Annotation::hasExperimentAnnotation() const { |
There was a problem hiding this comment.
I would move all this logic within the lowering from the AST to the MLIR domain, thus avoiding to have a dedicated AST node for the Experiment annotation (which doesn't map to any non-terminal of the grammar).
Actually, the same holds for the other kinds of specific annotations (e.g. the inverse function annotation). Eventually, they should all be moved to be part of the lowering logic (not your job, just saying).
| } | ||
|
|
||
| // Normally unreachable | ||
| assert(false); |
|
|
||
| // Normally unreachable | ||
| assert(false); | ||
| return ExperimentAnnotation(); |
There was a problem hiding this comment.
Prefer {} to default-construct.
| }; | ||
| } // namespace marco::ast::bmodelica | ||
|
|
||
| class ExperimentAnnotation { |
There was a problem hiding this comment.
The class is going to disappear as per other comments, but please pay attention to where you declare it. The header file is for functions.
2 participants
My implementation is for the most part derived from Annotation::getDerivativeAnnotation(), but I got it that this is not the preferred way of implementing such a feature. Could you point me in the right direction?
I also made some changes to the CMakeLists.txt this was done to make my LSP happy :D