-
Notifications
You must be signed in to change notification settings - Fork 170
[CIR] Add support for AliasOp #1925
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
base: main
Are you sure you want to change the base?
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the PR, besides comments, this also needs testcases and LLVM lowering support.
mlir::OpBuilder::InsertionGuard guard(builder); | ||
|
||
// Some global emissions are triggered while emitting a function, e.g. | ||
// void s() { x.method() } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This doesn't seem to apply here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
i think i need some help and explanation on this comment in the OG createCIRFunction
to understand more why it's not applicable.
// Be sure to insert a new function before a current one. | ||
auto *curCGF = getCurrCIRGenFun(); | ||
if (curCGF) | ||
builder.setInsertionPoint(curCGF->CurFn); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
is this ever the case? I can't see how one might want to insert an alias op outside the module level scope
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
i'm not quite sure what you mean here? don't we want to reset the insertion point if we're inside a function?
>; | ||
|
||
def CIR_AliasOp : CIR_Op<"alias", [AutomaticAllocationScope, CallableOpInterface, | ||
FunctionOpInterface, IsolatedFromAbove]> { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You need to add a description, examples, etc, see others for inspiration
/// Returns the region on the current operation that is callable. This may | ||
/// return null in the case of an external callable object, e.g. an external | ||
/// function. | ||
::mlir::Region *getCallableRegion() { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Will this operation also be used for global variable aliases?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
hi Andy, I want to have a simple lowering to LLVM_AliasOp but not sure how messy if the same op has two different functionalities. I want to try using 1 op first. I can change the comments
I've refactored the code to use CIRCallableOpInterface instead of FuncOp now but was wondering if it is ergonomic to do so. Right now if some person wants to use some methods that exists in FunctionOpInterface, they need to search up the return type and the argument type and add it to the CIRCallableOpInterface first, if they dont want to cast it to FunctionOpInterface. It stinks a bit but on top of my mind i can't see another cleaner way to do this. Only test case that doesn't pass is the test case that both requires alias as well as lowering to llvm. will handle that soon |
This is changing too much, and became way harder to review. What I was trying to suggest is that we shouldn't be duplicating code that can be covered with interfaces, meaning that if you are free to create a new one if it makes more sense, I'm basically alluding to the existing ones, it doesn't mean they have to be used. In LLVM, there are two cases for alias, function aliases and global aliases. I suggest you start a new PR looking at C/C++ examples that create aliases for both globals and function, and find out via that what it's the abstraction you really need. Right now, as you noticed, we have clunky support for alias functions in CIR, because LLVM dialect didn't have AliasOp until I added it some months ago, so you'd need to think about the design here. |
Related to #1913.
Added alias op and verifier for call op. Failing two tests related to constructor and destructor aliases
Right now
cir::FuncOp CIRGenModule::GetOrCreateCIRFunction
is being used in a lot of place where if we switch to using Alias in emitAliasForGlobal, we'll hit cast type error. I'm not quite sure if i should change the function result type to be mlir::Operation* or something that encompasses both function and alias or notPilot test case