Skip to content

Add polynomial.eval op for evaluation of a static polynomial#1271

Merged
copybara-service[bot] merged 1 commit intogoogle:mainfrom
zmbekdemir:poly_eval
Jan 23, 2025
Merged

Add polynomial.eval op for evaluation of a static polynomial#1271
copybara-service[bot] merged 1 commit intogoogle:mainfrom
zmbekdemir:poly_eval

Conversation

@zmbekdemir
Copy link
Contributor

This is the definition for polynomial.eval op. This operation builds without error but I am not sure if it is the final version. Insights and recommendations are appreciated. #1217

Copy link
Collaborator

@j2kun j2kun left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So the tablegen looks mostly good to me! A few small nits, and then I'd suggest adding a test using this new syntax in tests/Dialect/Polynomial/IR/ (could be in, say, int_coefficients.mlir or a new file if you prefer). This will ensure that heir-opt can run, parse+print the syntax you've chosen, as well as run any IR verifiers.

let description = [{
Evaluates the result of a polynomial specified as a static attribute at a given SSA value.
The result represents the evaluation of the polynomial at the input value and produces
a corresponding constant value.
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
a corresponding constant value.
a corresponding scalar value.

"constant" would imply statically known, and if the input is an SSA value then it's not in general.

);

let results = (outs AnyType:$result);
let assemblyFormat = "$polynomial attr-dict `polynomial` $value type($value) `->` type($result)";
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
let assemblyFormat = "$polynomial attr-dict `polynomial` $value type($value) `->` type($result)";
let assemblyFormat = "operands attr-dict `:` type($value)";

```mlir
#poly = #polynomial.int_polynomial<x**2 + 2*x + 1>
%x = arith.constant 5 : i32
%result = polynomial.eval #poly, %x : !arith.constant
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
%result = polynomial.eval #poly, %x : !arith.constant
%result = polynomial.eval #poly, %x : i32

@j2kun j2kun changed the title Add .td description for polynomial.eval op Add polynomial.eval op for evaluation of a static polynomial Jan 16, 2025
@j2kun
Copy link
Collaborator

j2kun commented Jan 23, 2025

@zmbekdemir if it's alright with you I can go ahead and cherry-pick your change and add my patches on top to submit it.

@zmbekdemir
Copy link
Contributor Author

@j2kun yes, go ahead please.

@j2kun j2kun added the pull_ready Indicates whether a PR is ready to pull. The copybara worker will import for internal testing label Jan 23, 2025
@j2kun
Copy link
Collaborator

j2kun commented Jan 23, 2025

Marking as pull_ready so I can patch it internally. The commit should still be attributed to you when it lands.

@copybara-service copybara-service bot merged commit 1420d98 into google:main Jan 23, 2025
@zmbekdemir zmbekdemir deleted the poly_eval branch February 11, 2025 06:41
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

pull_ready Indicates whether a PR is ready to pull. The copybara worker will import for internal testing

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants