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

pdl_interp: Sort constraints to end of predicate list. #474

Merged
merged 1 commit into from
Feb 21, 2025

Conversation

jorickert
Copy link

@jorickert jorickert commented Feb 18, 2025

When writing PDLL patterns it is often assumed that some basic checks are executed before constraints are called, but this is not always the case, as operations can be reordered in PDLInterp if there is no dependency between them.

For example:
Pdll pattern:

  let someOp = op<someDialect.SomeOp>(input: Value<inputTy: Type) {axis = inputAxis: Attr } -> (resTypes: TypeRange);
 let someResult = someConstraint(inputAxis);

If SomeOp requires axis to have a valid value, it is easy to (wrongly) assume that someConstraint always gets called with a not-null inputAxis. This is not correct.

The linearized PDLInterp (pseudo-code) could be the following:

%0 = pdl_interp.get_attribute "axis" of %arg0
%1 = pdl_interp.apply_constraint “someConstraint”(%0)
pdl_interp.is_not_null(%0)
pdl_interp.check_operation_name of %arg0 is "someDialect.SomeOp"

Note that here someConstraint can be called with a null attribute.

This commit changes the prioritization of predicates, so that constraints are run after other predicates.

%0 = pdl_interp.get_attribute "axis" of %arg0
pdl_interp.is_not_null(%0)
pdl_interp.check_operation_name of %arg0 is "someDialect.SomeOp"
%1 = pdl_interp.apply_constraint “someConstraint”(%0)

This ensures that null or operation name checks are run before constraints. This is closer to the mental model when writing PDLL patterns and should make it less likely to run into bugs caused by assuming implicit checks for not null.

@jorickert
Copy link
Author

jorickert commented Feb 18, 2025

  • Add/update tests
  • Check that there are no large performance regressions

@jorickert jorickert changed the title Sort constraints to end of predicate list. pdl_interp: Sort constraints to end of predicate list. Feb 18, 2025
@jorickert jorickert force-pushed the jrickert.constraints_prio branch from 58e78e5 to 71267ea Compare February 18, 2025 16:34
@jorickert jorickert marked this pull request as draft February 18, 2025 17:23
@jorickert jorickert force-pushed the jrickert.constraints_prio branch from 71267ea to 418ca00 Compare February 20, 2025 01:58
@jorickert jorickert marked this pull request as ready for review February 20, 2025 02:29
@jorickert jorickert removed the request for review from ttjost February 21, 2025 03:41
… are executed before constraints are called, but this is not always the case, as operations can be reordered in PDLInterp if there is no dependency between them.

For example:
Pdll pattern:
```
 let someOp = op<someDialect.SomeOp>(input: Value<inputTy: Type) {axis = inputAxis: Attr } -> (resTypes: TypeRange);
 let someResult = someConstraint(inputAxis);
```

If SomeOp requires axis to have a valid value, it is easy to (wrongly) assume that someConstraint always gets called with a not-null inputAxis. This is not correct.

The linearized PDLInterp (pseudo-code) could be the following:
```
%0 = pdl_interp.get_attribute "axis" of %arg0
%1 = pdl_interp.apply_constraint “someConstraint”(%0)
pdl_interp.is_not_null(%0)
pdl_interp.check_operation_name of %arg0 is "someDialect.SomeOp"
```

Note that here someConstraint can be called with a null attribute.

This commit changes the prioritization of predicates, so that constraints are run after other predicates.
```
%0 = pdl_interp.get_attribute "axis" of %arg0
pdl_interp.is_not_null(%0)
pdl_interp.check_operation_name of %arg0 is "someDialect.SomeOp"
%1 = pdl_interp.apply_constraint “someConstraint”(%0)
```
This ensures that null or operation name checks are run before constraints. This is closer to the mental model when writing PDLL patterns and should make it less likely to run into bugs caused by assuming implicit checks for not null.
@jorickert jorickert force-pushed the jrickert.constraints_prio branch from 418ca00 to 3c94045 Compare February 21, 2025 17:55
@jorickert jorickert enabled auto-merge February 21, 2025 17:56
@jorickert jorickert merged commit 91ca2e1 into feature/fused-ops Feb 21, 2025
4 checks passed
@jorickert jorickert deleted the jrickert.constraints_prio branch February 21, 2025 18:06
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