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.

Sign up for GitHub

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

Jump to bottom

[CIR][Codegen] RTTI support for virtual class inheritence #259

Merged

htyu merged 1 commit into llvm:main from htyu:vbase

Oct 18, 2023

Contributor

htyu commented Sep 14, 2023

This patch adds RTTI support for C++ virtual inheritance.

This patch does not include LLVM lowering support.

htyu requested a review from bcardosolopes

September 14, 2023 05:55

htyu mentioned this pull request

[CIR][Lowering] Lower vtable and type info #264

Merged

bcardosolopes requested changes

View reviewed changes

Member

bcardosolopes left a comment

Thanks for adding RTTI support for vbases

clang/lib/CIR/CodeGen/CIRGenItaniumCXXABI.cpp Outdated Show resolved Hide resolved

clang/lib/CIR/CodeGen/CIRGenItaniumCXXABI.cpp Outdated Show resolved Hide resolved

clang/lib/CIR/CodeGen/CIRGenItaniumCXXABI.cpp Outdated Show resolved Hide resolved

clang/lib/CIR/CodeGen/CIRGenItaniumCXXABI.cpp Show resolved Hide resolved

clang/lib/CIR/CodeGen/CIRGenItaniumCXXABI.cpp Outdated Show resolved Hide resolved

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

                     continue;
-                  emitError() << "expected GlobalViewAttr attribute";
+                  emitError() << "expected GlobalViewAttr or IntAttr attribute";

Member

bcardosolopes Sep 18, 2023

The fact that TypeInfo now supports a IntAttr seems a bit arbitrary in this PR, what does it mean? Can you add some docs to TypeInfoAttr?

Contributor Author

htyu Sep 20, 2023

The integers are for

  // Itanium C++ ABI 2.9.5p6c:
  //   __flags is a word with flags describing details about the class
  //   structure, which may be referenced by using the __flags_masks
  //   enumeration. These flags refer to both direct and indirect bases.
  unsigned Flags = ComputeVMIClassTypeInfoFlags(RD);
  Fields.push_back(mlir::cir::IntAttr::get(UnsignedIntLTy, Flags));

and

// Itanium C++ ABI 2.9.5p6c:
 //   __base_count is a word with the number of direct proper base class
 //   descriptions that follow.

Fields.push_back(mlir::cir::IntAttr::get(UnsignedIntLTy, RD->getNumBases()));

Updated doc for TypeInfoAttr

Member

bcardosolopes Sep 26, 2023

I was more curious about the meaning of those, which I just explained better in the other comment: use enum kind attributes for the flags.

Contributor Author

htyu Sep 29, 2023

We can use a enum attribute for the flags, but the int attribute is still needed for getNumBases?

clang/test/CIR/CodeGen/vbase.cpp Show resolved Hide resolved

htyu force-pushed the vbase branch from 0afd52e to 70c65d7 Compare

September 20, 2023 17:51

bcardosolopes requested changes

View reviewed changes

clang/include/clang/CIR/Dialect/IR/CIRAttrs.td

                   The verifier enforces that the output type is always a `!cir.struct`,
                   and that the ArrayAttr element types match the equivalent member type
-                  for the resulting struct.
+                  for the resulting struct, i.e, a GlobalViewAttr for symbol reference or
+                  an IntAttr for flags.

Member

bcardosolopes Sep 26, 2023

I still would prefer not to use an IntAttr but a enum kind attr that contains BCTI_Virtual, BCTI_Public, etc (and incrementally add the enums as we get usecases). Using IntAttr feels too opaque and already LLVM-like. Are there restrictions to doing such approach?

clang/lib/CIR/CodeGen/CIRGenItaniumCXXABI.cpp Outdated Show resolved Hide resolved

clang/lib/CIR/CodeGen/CIRGenItaniumCXXABI.cpp Show resolved Hide resolved

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

                     continue;
-                  emitError() << "expected GlobalViewAttr attribute";
+                  emitError() << "expected GlobalViewAttr or IntAttr attribute";

Member

bcardosolopes Sep 26, 2023

I was more curious about the meaning of those, which I just explained better in the other comment: use enum kind attributes for the flags.

clang/test/CIR/CodeGen/vbase.cpp Show resolved Hide resolved

bcardosolopes approved these changes

View reviewed changes

Member

bcardosolopes left a comment

LGTM, this is good to go, I'll file some issues so we can later improve specific bits.

bcardosolopes mentioned this pull request

Consider explicit mapping for BCTI_Virtual, BCTI_Public, etc #282

Open


          [CIR][Codegen] RTTI support for virtual class inheritence

f2efd27

htyu force-pushed the vbase branch from 70c65d7 to f2efd27 Compare

October 18, 2023 04:03

htyu merged commit fd263e4 into llvm:main

bcardosolopes pushed a commit that referenced this pull request


          [CIR][Lowering] Lower vtable and type info (#264)

b7bfef0

Lowering Vtable and RTTI globals. Also lowering AddressPoint.

based on #259

lanza pushed a commit that referenced this pull request


          [CIR][Codegen] RTTI support for virtual class inheritence (#259)

0f13afc

This patch adds RTTI support for C++ virtual inheritance. 

This patch does not include LLVM lowering support.

lanza pushed a commit that referenced this pull request


          [CIR][Lowering] Lower vtable and type info (#264)

65de671

Lowering Vtable and RTTI globals. Also lowering AddressPoint.

based on #259

lanza pushed a commit that referenced this pull request


          [CIR][Codegen] RTTI support for virtual class inheritence (#259)

ff787c4

This patch adds RTTI support for C++ virtual inheritance. 

This patch does not include LLVM lowering support.

lanza pushed a commit that referenced this pull request


          [CIR][Lowering] Lower vtable and type info (#264)

2a5d49f

Lowering Vtable and RTTI globals. Also lowering AddressPoint.

based on #259

lanza pushed a commit that referenced this pull request


          [CIR][Codegen] RTTI support for virtual class inheritence (#259)

affd1e5

This patch adds RTTI support for C++ virtual inheritance. 

This patch does not include LLVM lowering support.

lanza pushed a commit that referenced this pull request


          [CIR][Lowering] Lower vtable and type info (#264)

ea0aa03

Lowering Vtable and RTTI globals. Also lowering AddressPoint.

based on #259

lanza pushed a commit that referenced this pull request


          [CIR][Codegen] RTTI support for virtual class inheritence (#259)

1bdbb9a

This patch adds RTTI support for C++ virtual inheritance. 

This patch does not include LLVM lowering support.

lanza pushed a commit that referenced this pull request


          [CIR][Lowering] Lower vtable and type info (#264)

1363edd

Lowering Vtable and RTTI globals. Also lowering AddressPoint.

based on #259

lanza pushed a commit to lanza/llvm-project that referenced this pull request


          [CIR][Lowering] Lower vtable and type info (llvm#264)

d4a20d8

Lowering Vtable and RTTI globals. Also lowering AddressPoint.

based on llvm/clangir#259

lanza pushed a commit that referenced this pull request


          [CIR][Codegen] RTTI support for virtual class inheritence (#259)

ba8a72e

This patch adds RTTI support for C++ virtual inheritance. 

This patch does not include LLVM lowering support.

lanza pushed a commit that referenced this pull request


          [CIR][Lowering] Lower vtable and type info (#264)

5be2e86

Lowering Vtable and RTTI globals. Also lowering AddressPoint.

based on #259

eZWALT pushed a commit to eZWALT/clangir that referenced this pull request


          [CIR][Codegen] RTTI support for virtual class inheritence (llvm#259)

53ee1f3

This patch adds RTTI support for C++ virtual inheritance. 

This patch does not include LLVM lowering support.

eZWALT pushed a commit to eZWALT/clangir that referenced this pull request


          [CIR][Codegen] RTTI support for virtual class inheritence (llvm#259)

5bdf846

This patch adds RTTI support for C++ virtual inheritance. 

This patch does not include LLVM lowering support.

eZWALT pushed a commit to eZWALT/clangir that referenced this pull request


          [CIR][Lowering] Lower vtable and type info (llvm#264)

c3dff7a

Lowering Vtable and RTTI globals. Also lowering AddressPoint.

based on llvm#259

lanza pushed a commit that referenced this pull request


          [CIR][Codegen] RTTI support for virtual class inheritence (#259)

e8e0ef4

This patch adds RTTI support for C++ virtual inheritance. 

This patch does not include LLVM lowering support.

lanza pushed a commit that referenced this pull request


          [CIR][Lowering] Lower vtable and type info (#264)

5000dab

Lowering Vtable and RTTI globals. Also lowering AddressPoint.

based on #259

lanza pushed a commit that referenced this pull request


          [CIR][Codegen] RTTI support for virtual class inheritence (#259)

6369cd1

This patch adds RTTI support for C++ virtual inheritance. 

This patch does not include LLVM lowering support.

lanza pushed a commit that referenced this pull request


          [CIR][Lowering] Lower vtable and type info (#264)

e1dfda1

Lowering Vtable and RTTI globals. Also lowering AddressPoint.

based on #259

eZWALT pushed a commit to eZWALT/clangir that referenced this pull request


          [CIR][Codegen] RTTI support for virtual class inheritence (llvm#259)

6877c8e

This patch adds RTTI support for C++ virtual inheritance. 

This patch does not include LLVM lowering support.

eZWALT pushed a commit to eZWALT/clangir that referenced this pull request


          [CIR][Lowering] Lower vtable and type info (llvm#264)

acb0c8f

Lowering Vtable and RTTI globals. Also lowering AddressPoint.

based on llvm#259

lanza pushed a commit that referenced this pull request


          [CIR][Codegen] RTTI support for virtual class inheritence (#259)

1ef58ab

This patch adds RTTI support for C++ virtual inheritance.

This patch does not include LLVM lowering support.

lanza pushed a commit that referenced this pull request


          [CIR][Lowering] Lower vtable and type info (#264)

24ba261

Lowering Vtable and RTTI globals. Also lowering AddressPoint.

based on #259

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment