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

Bugfix: Change Syntax so that Parentheses are Required for llvm.mlir.constant #443

Merged
merged 9 commits into from
Jul 17, 2024

Conversation

AtticusKuhn
Copy link
Contributor

@AtticusKuhn AtticusKuhn commented Jul 9, 2024

Here is a reference in the LLVM manual:

llvm.mlir.constant (LLVM::ConstantOp)

operation ::= `llvm.mlir.constant` `(` $value `)` attr-dict `:` type($res)
// Integer constant, internal i32 is mandatory
%0 = llvm.mlir.constant(42 : i32) : i32

// It's okay to omit i64.
%1 = llvm.mlir.constant(42) : i64

// Floating point constant.
%2 = llvm.mlir.constant(42.0 : f32) : f32

// Splat dense vector constant.
%3 = llvm.mlir.constant(dense<1.0> : vector<4xf32>) : vector<4xf32>

In the llvm reference manual, parentheses are required after llvm.mlir.constant, but the syntax did not require parentheses. Thus, I changed the syntax.

It's my intuition that the Lean version of LLVM should stay as close to the real version of LLVM as possible.

This closes #440

change from
llvm.mlir.constant 8
to
llvm.mlir.constant(8) : i64
@AtticusKuhn AtticusKuhn changed the title Bugfox: Change S+yntax so that parens are required Bugfox: Change Syntax so that Parentheses are Required for llvm.mlir.constant Jul 9, 2024
@AtticusKuhn AtticusKuhn changed the title Bugfox: Change Syntax so that Parentheses are Required for llvm.mlir.constant Bugfix: Change Syntax so that Parentheses are Required for llvm.mlir.constant Jul 9, 2024
@tobiasgrosser
Copy link
Collaborator

Nice. There are some build failures, but this looks overall great. @alexkeizer might be able to comment on the lean code more.

Copy link

github-actions bot commented Jul 9, 2024

Alive Statistics: 58 / 93 (35 failed)

@alexkeizer
Copy link
Collaborator

Integer constant, internal i32 is mandatory
Interesting, I think we do allow the internal i32 to be omitted, as in

%0 = llvm.mlir.constant(42) : i32

Like you say, we should stick with the official grammar, so that's something we should probably address too (at some point, not necessarily in this PR)

Copy link
Collaborator

@alexkeizer alexkeizer left a comment

Choose a reason for hiding this comment

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

The build fails because there is another file, AliveAutoGenerated, which has a collection of mlir programs in the DSL too.
It's a bit slow, so we don't build it by default, you have to run lake build AliveExamples to explicitly build them.

As the name suggests, though, these examples are (mostly) automatically generated. Let's talk a bit more about how to fix this tomorrow

SSA/Projects/InstCombine/LLVM/PrettyEDSL.lean Outdated Show resolved Hide resolved
SSA/Projects/InstCombine/LLVM/PrettyEDSL.lean Outdated Show resolved Hide resolved
@tobiasgrosser
Copy link
Collaborator

I like this direction. @AtticusKuhn, it might be nice to close this on Monday.

@AtticusKuhn
Copy link
Contributor Author

I like this direction. @AtticusKuhn, it might be nice to close this on Monday.

I realised that I don't actually need this PR to do
#452
because I can use alternate syntax.
But still, I agree that it would be good to close this on Monday.

@tobiasgrosser
Copy link
Collaborator

Finishing this PR would be great, but don't hesitate to just close it if this is not relevant anymore.

Copy link

Alive Statistics: 63 / 93 (30 failed)

@AtticusKuhn AtticusKuhn requested a review from alexkeizer July 15, 2024 10:36
Copy link

Alive Statistics: 63 / 93 (30 failed)

@AtticusKuhn AtticusKuhn self-assigned this Jul 15, 2024
@AtticusKuhn AtticusKuhn added the bug Something isn't working label Jul 15, 2024
@alexkeizer
Copy link
Collaborator

I'm a bit hesitant to change AliveAutoGenerated manually. I think the best course of action at the moment is to just allow both the old syntax (without parens) alongside the proper LLVM syntax (with parens). That way, we can leave AliveAutoGenerated as is.

@AtticusKuhn
Copy link
Contributor Author

I'm a bit hesitant to change AliveAutoGenerated manually. I think the best course of action at the moment is to just allow both the old syntax (without parens) alongside the proper LLVM syntax (with parens). That way, we can leave AliveAutoGenerated as is.

That's probably a good idea. I could change the script that generates AliveAutoGenerated, but it's no extra work just to allow both syntaxes.

Copy link

Alive Statistics: 63 / 93 (30 failed)

@AtticusKuhn
Copy link
Contributor Author

Anyway, I've made the parentheses syntax optional, so both

    %c0 = llvm.mlir.constant 0

and

    %c0 = llvm.mlir.constant(0 : i32) : i32

are allowed

parenthesis syntax.
Copy link

Alive Statistics: 63 / 93 (30 failed)

Copy link
Collaborator

@alexkeizer alexkeizer left a comment

Choose a reason for hiding this comment

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

LGTM, a few minor comments left. Please address those and feel free to merge

SSA/Projects/InstCombine/LLVM/PrettyEDSL.lean Outdated Show resolved Hide resolved
SSA/Projects/InstCombine/LLVM/PrettyEDSL.lean Outdated Show resolved Hide resolved
SSA/Projects/InstCombine/PaperExamples.lean Outdated Show resolved Hide resolved
Copy link

Alive Statistics: 59 / 93 (34 failed)

@AtticusKuhn AtticusKuhn enabled auto-merge July 17, 2024 16:10
Copy link

Alive Statistics: 59 / 93 (34 failed)

@AtticusKuhn AtticusKuhn added this pull request to the merge queue Jul 17, 2024
Merged via the queue into main with commit 9b9fc9d Jul 17, 2024
2 checks passed
@AtticusKuhn AtticusKuhn deleted the bugfix/constantparens branch July 17, 2024 16:28
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Bug: LLVM syntax macro does not accept parentheses around llvm.mlir.constant but mlir-opt does
3 participants