Skip to content

Conversation

@fangyi-zhou
Copy link
Contributor

This PR contains 2 changes:

  • Add getters/setters for CIR visibility in CIRGlobalValueInterface, which is implemented by GlobalOp and FuncOp with linkage and CIR visibility. The getters/setters are analogous to existing getters/setters for linkage and symbol visibility.
  • Add a helper function for deducing MLIR visibility using both linkage and CIR visibility (previously only linkage was used).

more notes in #1029 (comment)


/// This function is used to deduce the MLIR visibility of a CIR symbol based on
/// CIR linkage and visibility.
static inline mlir::SymbolTable::Visibility
Copy link
Contributor Author

Choose a reason for hiding this comment

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

note: I'm not sure whether this is the best place to put this function. Let me know if I should move it somewhere else.

// TODO(cir): infer visibility from linkage in global op builder.
GV.setVisibility(getMLIRVisibilityFromCIRLinkage(Linkage));
GV.setVisibility(
cir::deduceMLIRVisibility(Linkage, cir::VisibilityKind::Default));
Copy link
Member

Choose a reason for hiding this comment

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

My suggestion is a bit more simple and somewhat different: GV.setLinkage and Fn.setLinkage should call GV.setVisibility under the hood, no need to involve visibility kind. If you make linkage into an interface, than you don't need to implement in both places.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks for the comment! I was trying to do that in future PRs, but I ran into some issues when dealing with FuncOp (#1029 (comment))

@lanza lanza force-pushed the main branch 2 times, most recently from d2c4ab8 to 8f89224 Compare July 23, 2025 17:04
@fangyi-zhou fangyi-zhou closed this Aug 3, 2025
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