Skip to content
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

[CIR] Track size_t and int size with module attributes #1389

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

AdUhTkJm
Copy link
Contributor

@AdUhTkJm AdUhTkJm commented Feb 22, 2025

To give LoweringPrepare type information from CIRGenTypeCache, this PR adds two attributes to ModuleOp:

module attributes {
  cir.int_size = #cir.int_size<32>,
  cir.size_type_size = #cir.size_type_size<64>,
  ...
} {}

The CIRDataLayout class is also extended to have getPtrDiffTy and so on.

Some tests that only expects cir.lang and cir.sob are also changed to take this into account.

@Lancern
Copy link
Member

Lancern commented Feb 24, 2025

A small question here: would it be better to provide all these type-layout related information within a single attribute, instead of spreading them into separate attributes?

For example, would it be better if we update this PR to instead provide a single attribute like this:

module attributes {
  cir.type_sizes = #cir.type_sizes<
    int = 32,
    size_t = 64,
  >,
} {}

@AdUhTkJm
Copy link
Contributor Author

AdUhTkJm commented Feb 24, 2025

A small question here: would it be better to provide all these type-layout related information within a single attribute, instead of spreading them into separate attributes?

For example, would it be better if we update this PR to instead provide a single attribute like this:

module attributes {
  cir.type_sizes = #cir.type_sizes<
    int = 32,
    size_t = 64,
  >,
} {}

That makes sense! Now I just changed it to be exactly like this.

@bcardosolopes
Copy link
Member

bcardosolopes commented Feb 24, 2025

@AdUhTkJm thanks for working on this - really good point @Lancern. My suggestion is that we do this for all types in CIRGenTypeCache that don't have an explicit bit-width attached to the name (FloatTy, DoubleTy, UIntPtrTy, SizeTy, etc). Few extra things:

  • We don't need to add an entry for all of them if unnecessary (e.g. unions in CIRGenTypeCache can be represented by the same entry but given two different getters in the attribute extraDeclarations).
  • Make sure you initialize the attribute before CIRGenTypeCache and then initialize the relevant members using the information from the attribute.
    The long term plan here (not for this PR) would be to kill CIRGenTypeCache in favor of only using the attribute in both CIRGen and elsewhere, at which point we can have more getters in the attribute to also populated the more obvious types, without storing any information like SInt8Ty, etc.

Copy link
Member

@bcardosolopes bcardosolopes left a comment

Choose a reason for hiding this comment

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

Comment above

// TypeSizeAttr
//===----------------------------------------------------------------------===//

def CIR_TypeSizesAttr : CIR_Attr<"TypeSizes", "type_sizes"> {
Copy link
Member

Choose a reason for hiding this comment

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

I suggest renaming this to TypeSizeInfo

```
}];

let parameters = (ins "unsigned":$intSize, "unsigned":$sizeTypeSize);
Copy link
Member

Choose a reason for hiding this comment

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

Please add extraClassDeclaration with static methods that return the full type, example: static mlir::Type getPtrDiffTy() { ... }


mlir::Type CIRDataLayout::getPtrDiffType(mlir::MLIRContext *ctx) const {
return cir::IntType::get(ctx, sizeTypeSize, /*signed=*/true);
}
Copy link
Member

Choose a reason for hiding this comment

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

If you do that above you don't need to add these.

Copy link
Member

@bcardosolopes bcardosolopes left a comment

Choose a reason for hiding this comment

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

Great, one more round of review

@@ -1,4 +1,5 @@
#include "clang/CIR/Dialect/IR/CIRDataLayout.h"
#include "clang/CIR/Dialect/IR/CIRDialect.h"
Copy link
Member

Choose a reason for hiding this comment

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

Is this include really needed?

// TypeSizesInfoAttr
//===----------------------------------------------------------------------===//

def CIR_TypeSizesInfoAttr : CIR_Attr<"TypeSizesInfo", "type_sizes_info"> {
Copy link
Member

Choose a reason for hiding this comment

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

CIR_TypeSizesInfoAttr -> CIR_TypeSizeInfoAttr
TypeSizesInfo -> TypeSizeInfo
type_sizes_info -> type_size_info

def CIR_TypeSizesInfoAttr : CIR_Attr<"TypeSizesInfo", "type_sizes_info"> {
let summary = "the size of types in bits";
let description = [{
The `cir.type_sizes` attribute is attached to a module, recording lengths
Copy link
Member

Choose a reason for hiding this comment

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

Update the description and example to match the new names


let parameters = (ins "unsigned":$char_size,
"unsigned":$int_size,
"unsigned":$size_type_size);
Copy link
Member

Choose a reason for hiding this comment

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

size_type_size -> size_t_size

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.

3 participants