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

[CIR][CIRGen] Removed extra space in "cir.shift( right)" (#997) #1009

Merged

Conversation

MarcoCalabretta
Copy link
Contributor

@MarcoCalabretta MarcoCalabretta commented Oct 26, 2024

The MLIR docs at https://mlir.llvm.org/docs/DefiningDialects/Operations/#literals specify that "An empty literal `` may be used to remove a space that is inserted implicitly after certain literal elements", so I inserted one before the right literal to remove the extra space that was being printed. Oddly, the bug is also fixed by inserting an empty literal after the `left` literal, which leads me to believe that tablegen is inserting an implicit space after the `left` literal.

Copy link
Collaborator

@smeenai smeenai left a comment

Choose a reason for hiding this comment

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

Thanks, this is a great catch!

The example in https://mlir.llvm.org/docs/DefiningDialects/Operations/#optional-else-group shows no spaces around the colon. Does that also work to remove the space here?

If it doesn't and you have to keep the diff as-is, could you link to https://mlir.llvm.org/docs/DefiningDialects/Operations/#literals in your PR description and quote the part about the meaning of an empty literal? That way people like me who aren't as familiar with ODS will be able to understand this change easily :)

@MarcoCalabretta
Copy link
Contributor Author

Thanks, this is a great catch!

The example in https://mlir.llvm.org/docs/DefiningDialects/Operations/#optional-else-group shows no spaces around the colon. Does that also work to remove the space here?

If it doesn't and you have to keep the diff as-is, could you link to https://mlir.llvm.org/docs/DefiningDialects/Operations/#literals in your PR description and quote the part about the meaning of an empty literal? That way people like me who aren't as familiar with ODS will be able to understand this change easily :)

Thanks for your comments!
I tried the example in https://mlir.llvm.org/docs/DefiningDialects/Operations/#optional-else-group. Unfortunately, that doesn't work to remove the space in " right". I think that the reason behind this extra space has to do with the fact that there are implicit spaces inserted after some literals, defined here: https://mlir.llvm.org/docs/DefiningDialects/Operations/#literals. The documentation is unclear about exactly when these implicit spaces are inserted so I can't be exactly sure what the problem is.

Thanks for your suggestion, I'll definitely update the PR description to make the reasoning behind the change clearer.

Copy link
Collaborator

@smeenai smeenai left a comment

Choose a reason for hiding this comment

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

Thanks for checking and updating the description!

@smeenai smeenai merged commit d7de21f into llvm:main Oct 28, 2024
7 checks passed
@MarcoCalabretta MarcoCalabretta deleted the circodegen-cleanup-remove-extra-asm-space branch October 28, 2024 10:02
lanza pushed a commit that referenced this pull request Nov 5, 2024
The MLIR docs at https://mlir.llvm.org/docs/DefiningDialects/Operations/#literals
specify that "An empty literal `` may be used to remove a space that is
inserted implicitly after certain literal elements", so I inserted one
before the `right` literal to remove the extra space that was being
printed. Oddly, the bug is also fixed by inserting an empty literal
_after_ the `left` literal, which leads me to believe that tablegen is
inserting an implicit space after the `left` literal.
bcardosolopes pushed a commit that referenced this pull request Nov 11, 2024
…1096)

Following #1009 and #1028, this PR removes the double white spaces in
the assembly format of `cir.global` op.

It's basically some `mlir-tablegen`-builtin magic: With
`constBuilderCall` specified, we can apply `DefaultValuedAttr` with any
default value we can construct from constant values. Then we can easily
omit the default in assembly. Hence, we don't need to compromise
anything for the wrapper attribute `cir::VisibilityAttr`.

Similarly to #1009, an empty literal ``` `` ``` is used to omit the
leading space emitted by inner attribute.

The test case `visibility-attribute.c` is modified to save the
intermediate CIR to disk and reflect the effects.

Double whitespaces in other test files are removed.
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.

2 participants