Details
- Reviewers
nicolasvasilache rriddle mehdi_amini
Diff Detail
- Repository
- rG LLVM Github Monorepo
Event Timeline
mlir/lib/Dialect/Func/Transforms/DuplicateFunctionElimination.cpp | ||
---|---|---|
74 | There shouldn't be anything special about "ModuleOp", I suspect you probably want to walk operations that define a symbol table? | |
76 | Aren't there other potential uses of a function? I would think that there are operations taking the address of a function for example. How does the Inliner finds all uses of a symbol? | |
78 | I don't understand what is this check intended to catch? Nested modules? I would walk all Operations, check if the operation defined a SymbolTable, and return WalkResult::Skip to avoid walking through it. |
Addrwss comments
mlir/lib/Dialect/Func/Transforms/DuplicateFunctionElimination.cpp | ||
---|---|---|
76 | Right, there may be other uses. For now, we only need call sites in XLA:CPU and I don't see an interface for this. I suggest we add them as needed. Otherwise, I would probably move this pass to the XLA repo for now. | |
78 | Yes, this is for nested modules. Skip did not work as expected for some reason :/ |
mlir/lib/Dialect/Func/Transforms/DuplicateFunctionElimination.cpp | ||
---|---|---|
76 | I don’t quite get your point here: I see this as an actual bug making the pass unsafe to use, you seem to think this is some « nice to have » extra features or something? | |
95 | Without skip the operation will be traversed incorrectly, won’t it? |
mlir/lib/Dialect/Func/Transforms/DuplicateFunctionElimination.cpp | ||
---|---|---|
76 | Yeah, that's kind of a serious bug right now. That should be handled here, otherwise the pass isn't really usable outside of very narrow situations. | |
78 | Default walk is post-order, i.e. you walk the inner operations first (and skip is meaningless). You need a walk<PreOrder> for that to be useful. |
Haven't forgotten about this but, unfortunately, there were more urgent things to do.
If you prefer, I can move the pass to XLA for now and move it here when I find the time.
mlir/lib/Dialect/Func/Transforms/DuplicateFunctionElimination.cpp | ||
---|---|---|
76 | Why is that a bug? The old functions remain untouched as we no longer delete them. | |
95 | It would have skipped all the irrelevant ops. With WalkOrder::PreOrder this works and is a bit nicer, I guess. |
It's up to you actually, there is just a bit more time pressure to address post-review comment because code becomes a bit more "load bearing" after it lands. I'd think that if you can fix this in the coming week it is fine.
mlir/lib/Dialect/Func/Transforms/DuplicateFunctionElimination.cpp | ||
---|---|---|
76 |
Ah you may very well be right!
I thought we had such an interface? |
There shouldn't be anything special about "ModuleOp", I suspect you probably want to walk operations that define a symbol table?