[CIR] Add `cir.dyn_cast` operation #483

Lancern · 2024-02-25T15:34:43Z

This PR adds the cir.dyn_cast operation for representing dynamic_cast in C++. It contains the following contents:

A new cir.dyn_cast operation.
~~Two new attributes that will be attached to cir.dyn_cast operations:~~
- ~~#cir.dyn_cast_info attributes, which gives general information about a dynamic cast (e.g. the source RTTI pointer, the dest RTTI pointer, etc.)~~
- #cir.downcast_info attribute, which gives even more detailed information about a dynamic cast that is a down-cast. These information will be used when rewriting the cir.dyn_cast operation with more fundamental CIR operations.
CIRGen support for the new operation and attributes.
Rewrite the new operation with more fundamental CIR operations in LoweringPrepare.

This is a draft PR. Now I only added the new operation / attributes, and updated the CIRGen part. The LoweringPrepare for the new operation is not implemented. Hopefully the draft can get some initial feedbacks from the community and make sure it is on the right direction so we don't waste time on wrong things.

Related issue: #470 .

Lancern · 2024-04-07T14:24:20Z

This PR is finally complete and I believe it's ready for some reviews. Here is a brief of what is included in this PR (copied from the commit message).

This PR adds the cir.dyn_cast operation to model the C++ dynamic_cast operator. CIRGen code for the new operation is also included. During LLVMIR lowering prepare, the operation will be replaced by more basic CIR operations.

The new operation is attached an attribute that contains ABI information about the cast. During CIRGen, CIRGenCXXABI will generate and populate the attribute. During LLVMIR lowering prepare, the attribute will be used to generate code for the operation. The attribute's type is ABI-specific. This PR includes a #cir.itanium_dyn_cast_info attribute that provides Itanium C++ ABI specific information.

Since the code generated by the new operation is also highly ABI-specific, this PR adds a new class LoweringPrepareCXXABI that provides ABI-specific behaviors for lowering prepare. Similar to CIRGenCXXABI, the class is an abstract base class that concrete ABIs can inherit and implement. This PR includes a LoweringPrepareItaniumCXXABI class that provides Itanium C++ ABI behaviors for lowering prepare.

bcardosolopes

Awesome, thank you for updating the PR.

Overall approach looks great, it's getting close! Few comments inline:

I don't think we need LoweringPrepareItaniumCXXABI abstration during LoweringPrepare. It's fine if the dynamic_cast operation already contains everything it needs w.r.t to the Itanium ABI, whenever support is added for other ABIs, the operation can be changed to support that, or a new operation could also be created that makes more sense. Let's not over design to cover a use case we don't fully understand right now.
- Note that you are already lowering offsetHint early enough, and you should also do more of that in case there's anything blocking removal of said LoweringPrepareItaniumCXXABI.
ItaniumDynamicCastInfoAttr seems nice, you could add more information about Itanium ABI in the comments, but I rather we name it more regularly for now (same rationale as above): DynCastInfoAttr + #cir.dyncast_info sounds good enough.
Since you are adding verification, please add a dyncast.cir test to exercise the verifiers.

clang/lib/CIR/Dialect/IR/CIRAttrs.cpp

clang/lib/CIR/Dialect/Transforms/LoweringPrepareItaniumCXXABI.cpp

clang/lib/CIR/Dialect/Transforms/LoweringPrepare.cpp

clang/test/CIR/CodeGen/dynamic-cast.cpp

Lancern · 2024-04-17T16:01:41Z

Rebased onto the latest main.

bcardosolopes

This looks great, some remaining minor issues and should be good to go

clang/lib/CIR/Dialect/Transforms/LoweringPrepareItaniumCXXABI.cpp

clang/lib/CIR/CodeGen/CIRGenItaniumCXXABI.cpp

clang/lib/CIR/Dialect/IR/MissingFeatures.h

clang/lib/CIR/Dialect/Transforms/LoweringPrepareItaniumCXXABI.cpp

bcardosolopes · 2024-04-17T18:09:46Z

clang/lib/CIR/Dialect/Transforms/LoweringPrepareItaniumCXXABI.cpp

+
+  auto dynCastFuncRef = castInfo.getRuntimeFunc();
+  mlir::Value dynCastFuncArgs[4] = {srcPtr, srcRtti, destRtti, offsetHint};
+  // TODO(cir): set the calling convention for __dynamic_cast.


@sitio-couto FYI. How is this going to work with your CC work?

This patch adds the `cir.dyn_cast` operation to model the C++ `dynamic_cast` operator. CIRGen code for the new operation is also included. During LLVMIR lowering prepare, the operation will be replaced by more basic CIR operations. The new operation is attached an attribute named `#cir.dyn_cast_info` that contains ABI information about the cast. During CIRGen, `CIRGenCXXABI` will generate and populate the attribute. During LLVMIR lowering prepare, the attribute will be used to generate code for the operation. Since the code generated by the new operation is also highly ABI-specific, this patch adds a new class `LoweringPrepareCXXABI` that provides ABI-specific behaviors for lowering prepare. Similar to `CIRGenCXXABI`, the class is an abstract base class that concrete ABIs can inherit and implement. This patch includes a `LoweringPrepareItaniumCXXABI` class that provides Itanium C++ ABI behaviors for lowering prepare.

Lancern · 2024-04-18T14:28:15Z

Rebased onto the latest main.

bcardosolopes

LGTM

This PR adds the `cir.dyn_cast` operation for representing `dynamic_cast` in C++. It contains the following contents: - [x] A new `cir.dyn_cast` operation. - [x] ~Two new attributes that will be attached to `cir.dyn_cast` operations:~ - [x] ~`#cir.dyn_cast_info` attributes, which gives general information about a dynamic cast (e.g. the source RTTI pointer, the dest RTTI pointer, etc.)~ - [x] ~`#cir.downcast_info` attribute, which gives even more detailed information about a dynamic cast that is a down-cast. These information will be used when rewriting the `cir.dyn_cast` operation with more fundamental CIR operations.~ - [x] CIRGen support for the new operation and attributes. - [x] Rewrite the new operation with more fundamental CIR operations in LoweringPrepare. ~This is a draft PR. Now I only added the new operation / attributes, and updated the CIRGen part. The LoweringPrepare for the new operation is not implemented. Hopefully the draft can get some initial feedbacks from the community and make sure it is on the right direction so we don't waste time on wrong things.~ Related issue: #470 .

This PR adds the `cir.dyn_cast` operation for representing `dynamic_cast` in C++. It contains the following contents: - [x] A new `cir.dyn_cast` operation. - [x] ~Two new attributes that will be attached to `cir.dyn_cast` operations:~ - [x] ~`#cir.dyn_cast_info` attributes, which gives general information about a dynamic cast (e.g. the source RTTI pointer, the dest RTTI pointer, etc.)~ - [x] ~`#cir.downcast_info` attribute, which gives even more detailed information about a dynamic cast that is a down-cast. These information will be used when rewriting the `cir.dyn_cast` operation with more fundamental CIR operations.~ - [x] CIRGen support for the new operation and attributes. - [x] Rewrite the new operation with more fundamental CIR operations in LoweringPrepare. ~This is a draft PR. Now I only added the new operation / attributes, and updated the CIRGen part. The LoweringPrepare for the new operation is not implemented. Hopefully the draft can get some initial feedbacks from the community and make sure it is on the right direction so we don't waste time on wrong things.~ Related issue: llvm#470 .

This PR adds the `cir.dyn_cast` operation for representing `dynamic_cast` in C++. It contains the following contents: - [x] A new `cir.dyn_cast` operation. - [x] ~Two new attributes that will be attached to `cir.dyn_cast` operations:~ - [x] ~`#cir.dyn_cast_info` attributes, which gives general information about a dynamic cast (e.g. the source RTTI pointer, the dest RTTI pointer, etc.)~ - [x] ~`#cir.downcast_info` attribute, which gives even more detailed information about a dynamic cast that is a down-cast. These information will be used when rewriting the `cir.dyn_cast` operation with more fundamental CIR operations.~ - [x] CIRGen support for the new operation and attributes. - [x] Rewrite the new operation with more fundamental CIR operations in LoweringPrepare. ~This is a draft PR. Now I only added the new operation / attributes, and updated the CIRGen part. The LoweringPrepare for the new operation is not implemented. Hopefully the draft can get some initial feedbacks from the community and make sure it is on the right direction so we don't waste time on wrong things.~ Related issue: #470 .

lanza force-pushed the main branch 2 times, most recently from ed3955a to 8b7417c Compare March 23, 2024 05:07

lanza force-pushed the main branch from 8b7417c to 330c983 Compare March 30, 2024 04:29

Lancern force-pushed the dynamic-cast-op branch from 6db25e3 to c831bdf Compare April 7, 2024 14:19

Lancern marked this pull request as ready for review April 7, 2024 14:21

Lancern changed the title ~~[WIP][CIR] Add cir.dynamic_cast operation~~ [WIP][CIR] Add cir.dyn_cast operation Apr 7, 2024

Lancern changed the title ~~[WIP][CIR] Add cir.dyn_cast operation~~ [CIR] Add cir.dyn_cast operation Apr 7, 2024

bcardosolopes requested changes Apr 9, 2024

View reviewed changes

lanza force-pushed the main branch from 93715bd to a8b0ecd Compare April 17, 2024 05:15

Lancern force-pushed the dynamic-cast-op branch from c831bdf to a30b776 Compare April 17, 2024 15:52

bcardosolopes requested changes Apr 17, 2024

View reviewed changes

Lancern added 2 commits April 18, 2024 22:27

Fix some nits as commented in the reviews

3fef0fa

Lancern force-pushed the dynamic-cast-op branch from f231dba to 3fef0fa Compare April 18, 2024 14:28

bcardosolopes approved these changes Apr 19, 2024

View reviewed changes

bcardosolopes merged commit 8a322b7 into llvm:main Apr 19, 2024
6 checks passed

Lancern deleted the dynamic-cast-op branch April 19, 2024 04:07

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[CIR] Add `cir.dyn_cast` operation #483

[CIR] Add `cir.dyn_cast` operation #483

Lancern commented Feb 25, 2024 •

edited

Loading

Lancern commented Apr 7, 2024

bcardosolopes left a comment

Lancern commented Apr 17, 2024

bcardosolopes left a comment

bcardosolopes Apr 17, 2024

Lancern commented Apr 18, 2024

bcardosolopes left a comment

[CIR] Add cir.dyn_cast operation #483

[CIR] Add cir.dyn_cast operation #483

Conversation

Lancern commented Feb 25, 2024 • edited Loading

Lancern commented Apr 7, 2024

bcardosolopes left a comment

Choose a reason for hiding this comment

Lancern commented Apr 17, 2024

bcardosolopes left a comment

Choose a reason for hiding this comment

bcardosolopes Apr 17, 2024

Choose a reason for hiding this comment

Lancern commented Apr 18, 2024

bcardosolopes left a comment

Choose a reason for hiding this comment

[CIR] Add `cir.dyn_cast` operation #483

[CIR] Add `cir.dyn_cast` operation #483

Lancern commented Feb 25, 2024 •

edited

Loading