Kernel IR refactoring: part 5.1#290
Conversation
This reverts commit fc09c1b5a7240701da093406753908eba6f41e1d.
csarofeen
left a comment
There was a problem hiding this comment.
Seems reasonable, minor comments, pre-approved.
| namespace fuser { | ||
| namespace kir { | ||
|
|
||
| #if 0 |
There was a problem hiding this comment.
Is there a reason to leave this in the PR?
There was a problem hiding this comment.
Good catch - no good reason. It's likely going to be back in the next iteration but I will remove it from this PR.
|
|
||
| void handle(const TensorDomain* node) override { | ||
| const auto lowered_node = new kir::TensorDomain(node); | ||
| TORCH_CHECK(gpu_lower_->kir_map_.insert({node, lowered_node}).second); |
There was a problem hiding this comment.
Why not put gpu_lower_->kir_map_.insert({node, lowered_node}).second in the constructor new kir::TensorDomain(node)
I'm worried people could call kir::TensorDomain(node) directly instead of using loweredValue(node) which would typically be a bad thing to do.
There was a problem hiding this comment.
kir::TensorDomain's constructor doesn't know about GpuLower (or have access to kir_map_). Or have I misunderstood the comment?
I do agree about the potential trouble with people calling the "lowering" constructors directly though. I'm still considering options to limit this, but I haven't spent too much time on it since we need to revisit the whole Kernel IR "builder" interface soon anyway (once it's a separate class hierarchy, we're not going to have the luxury of FusionGuard or Fusion::registerStuff())
There was a problem hiding this comment.
Sounds good, I'm hopeful we can limit it, I got in some trouble when they were called explicitly before, would be nice to hide them if we can.
We're one step closer to a specialized kernel IR container. This iteration is restructuring some of the state in preparation for this standalone container: