Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

pattern matching #766

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

pattern matching #766

wants to merge 1 commit into from

Conversation

caleridas
Copy link
Collaborator

No description provided.

@caleridas
Copy link
Collaborator Author

@phate just as a proposal

I am experimenting with ways to unify the various different ways that the code presently does "pattern-matching" over operators and other things.

This way ensures that the "match" branch always receives the correct type etc.

@phate
Copy link
Owner

phate commented Jan 23, 2025

@haved Your opinion would be appreciated.

@phate
Copy link
Owner

phate commented Jan 23, 2025

@caleridas I like the idea, even though I have to say that I hate c++ shenanigan. It is utterly unreadable. One follow up question: This would currently not work if I would have to hand in more than just the operation to one of those function, right? Would the solution be here to bind the arguments first to these functions (or just use lambdas) such that only the operation would be left?

I can kind of see how this could be used to implement generalized/configurable optimizations. Take dead node elimination for example. One would like to have the general algorithm lined out, but then just add support for the individual nodes one would like to handle. In case of LLVM, this would only be theta, gamma, lambda, etc., but in case of HLS, the loop node would need to come on top of that. Thus, it would be nice if one could configure support for nodes/operations into a transformation. This here is the first step towards it, I believe.

@haved
Copy link
Collaborator

haved commented Jan 29, 2025

@phate @caleridas I see that LLVM used to have something called dyn_switch, which looked like the following:

dyn_switch(V->stripPointerCasts(),
           [] (PHINode *PN) {
             // process phis...
           },
           [] (SelectInst *SI) {
             // process selects...
           },
           [] (LoadInst *LI) {
             // process loads...
           },
           [] (AllocaInst *AI) {
             // process allocas...
           });

but it no longer appears to exist, and they have moved on to TypeSwitch, which is a class that is used like so:

Operation *op = ...;
LogicalResult result = TypeSwitch<Operation *, LogicalResult>(op)
  .Case<ConstantOp>([](ConstantOp op) { ... })
  .Default([](Operation *op) { ... });

Its source code can be seen here.
There is a fair bit of C++ template magic, and a specialization for returning void.

For our own code, I kind of like the pattern_match.hpp shown in this PR better, as it is shorter. One downside is that pattern_match does not support return values.

In any case I would want the pattern_match.hppto be in util/, and not rvsdg/.

Note that LLVM's Case methods are marked ALAWAYS_INLINE as well as NO_DEBUG, to prevent overhead, and prevent stack traces from containing a lot of Case. We probably want to do the same.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants