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][Dialect][NFC] Fix double whitespaces in cir.func assembly #1028

Merged
merged 1 commit into from
Oct 30, 2024

Conversation

seven-mile
Copy link
Collaborator

This PR fixes the notorious double whitespaces introduced by visibility attribute, for cir.func only.

It uses "leading whitespace" for every print. And the printing of visibility attr is properly guarded by a check of !isDefault().

Double whitespaces in test files are removed.

@seven-mile
Copy link
Collaborator Author

seven-mile commented Oct 30, 2024

Note that cir.global still has a similar problem, but for a little different reason.

cir.global uses MLIR assembly format with custom parser and printer custom<OmitDefaultVisibility>($global_visibility). When global_visibility has the enum case Default, it should not be printed. Thus, the custom printer function does nothing in this case. But an extra whitespace is still automatically emitted by MLIR assembly format generated code, resulting in double whitespaces.

Usually the best practice is to use the combination of I32EnumAttr and DefaultValuedAttr<>, which is recognized by assembly format and ensures the attribute is not printed for its default value (ref). Then, we can directly use DefaultValuedAttr<GlobalLinkageKind, "GlobalLinkageKind::ExternalLinkage"> and everything works.

But currently global_visibility in cir.global has type VisibilityAttr, which is a shallow wrapper of the underlying I32EnumAttr. This wrapper makes it hard to provide such a default value: we need a runtime MLIR context to build a default value.

Two thoughts on it:

  • Replace VisibilityAttr with VisibilityKind, which is I32EnumAttr, or
  • Use a null attribute as the default value of VisibilityAttr
    • Not ideal semantically, because default visibility != absence of visibility.

@bcardosolopes
Copy link
Member

@seven-mile thanks a bunch for working on this, I filled #1029 with more details on what was the plan here.

Two thoughts on it

Which one do you think help us get to #1029 faster?

@bcardosolopes bcardosolopes merged commit fccd337 into llvm:main Oct 30, 2024
7 checks passed
lanza pushed a commit that referenced this pull request Nov 5, 2024
)

This PR fixes the notorious double whitespaces introduced by visibility
attribute, for `cir.func` only.

It uses "leading whitespace" for every print. And the printing of
visibility attr is properly guarded by a check of `!isDefault()`.

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