llvm / clangir

A new (MLIR based) high-level IR for clang.
https://clangir.org
Other
308 stars 84 forks source link

[CIR] Add FuncAttrs to cir.calls #637

Closed roro47 closed 3 weeks ago

roro47 commented 1 month ago

Some function attributes are also callsite attributes, for instance, nothrow. This means they are going to show up in both. We don't support that just yet, hence the PR.

CIR has an attribute ExtraFuncAttr that we current use as part of FuncOp, see CIROps.td. This attribute also needs to be added to CallOp and TryCalOp.

Right now, In CIRGenCall.cpp: AddAttributesFromFunctionProtoType fills in FuncAttrs, but doesn't use it for anything. We should use the FuncAttrs result to populate constructing a ExtraFuncAttr and add it to the aforementioned call operations.

roro47 commented 1 month ago

@bcardosolopes Currently when running ninja check-clang-cir, 8 test cases are failing from the new changes because of

'cir.call' op requires attribute extra_attrs

This is because CallOp are created in other places outside of CIRGenCall.cpp with cir::CIRGenBuilderTy.create<mlir::cir::CallOp> which didn't handle passing extra_attrs. I think the best way to fix this is to modify those calls so that they all use just the methods from CIRGenCall.cpp.

It makes sense to only use methods in CIRGenCall.cpp to create callop instead of directly calling cir::CIRGenBuilderTy.create<mlir::cir::CallOp>. What do you think about this approach?

bcardosolopes commented 1 month ago

Thanks for your first PR, this is great!

This is because CallOp are created in other places outside of CIRGenCall.cpp with cir::CIRGenBuilderTy.create<mlir::cir::CallOp> which didn't handle passing extra_attrs. I think the best way to fix this is to modify those calls so that they all use just the methods from CIRGenCall.cpp.

Sounds like a good plan!

It makes sense to only use methods in CIRGenCall.cpp to create callop instead of directly calling cir::CIRGenBuilderTy.create<mlir::cir::CallOp>. What do you think about this approach?

I always prefer to refactor common code and boil down to one place that actually does the final calls to builder.create<...CallOp..>. But if it's too complex to tackle all existing ones, you can start incremental and only change existing helpers to pass down extra_attrs, and in a later PR you do further refactoring.

roro47 commented 1 month ago

Thanks for your first PR, this is great!

This is because CallOp are created in other places outside of CIRGenCall.cpp with cir::CIRGenBuilderTy.create<mlir::cir::CallOp> which didn't handle passing extra_attrs. I think the best way to fix this is to modify those calls so that they all use just the methods from CIRGenCall.cpp.

Sounds like a good plan!

It makes sense to only use methods in CIRGenCall.cpp to create callop instead of directly calling cir::CIRGenBuilderTy.create<mlir::cir::CallOp>. What do you think about this approach?

I always prefer to refactor common code and boil down to one place that actually does the final calls to builder.create<...CallOp..>. But if it's too complex to tackle all existing ones, you can start incremental and only change existing helpers to pass down extra_attrs, and in a later PR you do further refactoring.

I didn't see any extra helper that was calling create<milr::cir::CallOp> so I added a new helper called createCallOp to replace it.

bcardosolopes commented 3 weeks ago

Looks like there are some failing tests, possibly some refactoring went wrong. You can catch those locally by running ninja check-clang-cir.