-
Notifications
You must be signed in to change notification settings - Fork 79
Description
As noted in the issue #2404, I made a mistake in modifying the IR when implementing #2236. I replaced the definition of a TensorView that had a Reduction axis, with a cast op. Normally the cast op should strip the reduction of its inputs, so there should never be a reduction axis in the output of such an op. Since this went unchecked, there was a silent bug; the output was correct but an extra loop was generated, killing perf.
I propose that we introduce virtual void Expr::validate(const std::vector<Val*>& inputs, const std::vector<Val*>& outputs, const std::vector<Val*>& attributes) const. This would be called when constructing a new Expr, and also when we are replacing the definition of a val; i.e. in newObjectFunc. That way, we will get the same validation when doing surgery on the IR that we get when creating IR nodes. There still may be some hard-coded assertions in the actual constructors but we should try and keep those to a minimum in this new system to avoid unchecked violations.
Related: checkConcretization
We currently already have an interface to check that a single input or output is a valid concretization:
Line 1378 in ad7ea44
| void SqueezeOp::checkConcretization(Val* old_val, Val* new_val) const { |
This is currently only used by SqueezeOp to verify that we don't concretize squeezed dims to Iteration.
We could remove checkConcretization altogether since validation will happen when the concretization is finalized in this new approach.