This is an archive of the discontinued LLVM Phabricator instance.

[ODRHash] Drive attribute hashing through TableGen. NFC intended.
Needs ReviewPublic

Authored by vsapsai on Nov 28 2022, 1:15 PM.

Details

Summary

In this change deliberately keeping the tests intact to make sure
TableGen implementation works the same way as the manual one.

Diff Detail

Event Timeline

vsapsai created this revision.Nov 28 2022, 1:15 PM
Herald added a project: Restricted Project. · View Herald TranscriptNov 28 2022, 1:15 PM
Herald added a subscriber: ributzka. · View Herald Transcript
vsapsai requested review of this revision.Nov 28 2022, 1:15 PM
Herald added a project: Restricted Project. · View Herald TranscriptNov 28 2022, 1:15 PM
Herald added a subscriber: cfe-commits. · View Herald Transcript

I dont have a concern on this in general, but does it cause problems with modules built? I would think this flag needs to be written in ASTWriter/picked back up.

This patch builds on top of D135472 and aims to show that hashing attributes with TableGen works fine and doesn't have unexpected problems.

As the next step I plan to hash more attributes. Though there is one question regarding arguments excluded from the hash, for example, deprecation message. My plan for that is to introduce a separate attribute tentatively called ExcludeFromHashingArguments. I've decided not to repeat hashable pieces for each attribute but only to exclude a few of them as I expect it provide more signal.

I dont have a concern on this in general, but does it cause problems with modules built? I would think this flag needs to be written in ASTWriter/picked back up.

For an attribute attached to a Decl, attribute's hash contributes to Decl's hash which is serialized/deserialized (D135472 has more details).

The hash there isn't the problem, its that you're adding a field to Attr.td that isn't serialized in ASTWriter/ASTReader.

The hash there isn't the problem, its that you're adding a field to Attr.td that isn't serialized in ASTWriter/ASTReader.

That's a good point. Sorry I haven't realized it at first and thanks for your patience. So, IsODRHashable is a property of the attribute kind, not the attribute instance. Unfortunately, I don't know what is the appropriate way to fix FIXME below (and how urgent it is).

// FIXME: These are properties of the attribute kind, not state for this
// instance of the attribute.
...
unsigned IsODRHashable : 1;

Anyway, big chunk of attribute deserialization happens in "AttrPCHRead.inc" and for AMDGPUFlatWorkGroupSizeAttr (non-trivial example) we have

case attr::AMDGPUFlatWorkGroupSize: {
  bool isInherited = Record.readInt();
  bool isImplicit = Record.readInt();
  bool isPackExpansion = Record.readInt();
  Expr * min = Record.readExpr();
  Expr * max = Record.readExpr();
  New = new (Context) AMDGPUFlatWorkGroupSizeAttr(Context, Info, min, max);
  cast<InheritableAttr>(New)->setInherited(isInherited);
  New->setImplicit(isImplicit);
  New->setPackExpansion(isPackExpansion);
  break;
}

which calls the generated constructor

AMDGPUFlatWorkGroupSizeAttr::AMDGPUFlatWorkGroupSizeAttr(ASTContext &Ctx, const AttributeCommonInfo &CommonInfo
              , Expr * Min
              , Expr * Max
             )
  : InheritableAttr(Ctx, CommonInfo, attr::AMDGPUFlatWorkGroupSize, false, false, false)
              , min(Min)
              , max(Max)
  {
}

where false, false, false part corresponds to bool IsLateParsed, bool IsODRHashable, bool InheritEvenIfAlreadyPresent. So IsODRHashable is never serialized/deserialized and compiler knows what the value should be. It can be a problem if one clang version writes a .pcm file and another version reads it because we can change if an attribute is hashable over time. But as far as I know, we don't support such cross-version scenario already.

The hash there isn't the problem, its that you're adding a field to Attr.td that isn't serialized in ASTWriter/ASTReader.

That's a good point. Sorry I haven't realized it at first and thanks for your patience. So, IsODRHashable is a property of the attribute kind, not the attribute instance. Unfortunately, I don't know what is the appropriate way to fix FIXME below (and how urgent it is).

// FIXME: These are properties of the attribute kind, not state for this
// instance of the attribute.
...
unsigned IsODRHashable : 1;

Anyway, big chunk of attribute deserialization happens in "AttrPCHRead.inc" and for AMDGPUFlatWorkGroupSizeAttr (non-trivial example) we have

case attr::AMDGPUFlatWorkGroupSize: {
  bool isInherited = Record.readInt();
  bool isImplicit = Record.readInt();
  bool isPackExpansion = Record.readInt();
  Expr * min = Record.readExpr();
  Expr * max = Record.readExpr();
  New = new (Context) AMDGPUFlatWorkGroupSizeAttr(Context, Info, min, max);
  cast<InheritableAttr>(New)->setInherited(isInherited);
  New->setImplicit(isImplicit);
  New->setPackExpansion(isPackExpansion);
  break;
}

which calls the generated constructor

AMDGPUFlatWorkGroupSizeAttr::AMDGPUFlatWorkGroupSizeAttr(ASTContext &Ctx, const AttributeCommonInfo &CommonInfo
              , Expr * Min
              , Expr * Max
             )
  : InheritableAttr(Ctx, CommonInfo, attr::AMDGPUFlatWorkGroupSize, false, false, false)
              , min(Min)
              , max(Max)
  {
}

where false, false, false part corresponds to bool IsLateParsed, bool IsODRHashable, bool InheritEvenIfAlreadyPresent. So IsODRHashable is never serialized/deserialized and compiler knows what the value should be. It can be a problem if one clang version writes a .pcm file and another version reads it because we can change if an attribute is hashable over time. But as far as I know, we don't support such cross-version scenario already.

The explanation makes sense to me.

And I think we should add tests for this. e.g., in the ASTImporterTests.cpp.

clang/lib/AST/AttrImpl.cpp
16

Is this change necessary?

aaron.ballman added inline comments.Nov 29 2022, 10:06 AM
clang/include/clang/Basic/Attr.td
552–553

Do we want to change the default for any of the derived classes? e.g., should something that is a TypeAttr or DeclOrTypeAttr default to being ODR hashable because they impact the type system?

Also, can we expand the comment somewhat to help folks understand the circumstances under which they should set that to 1?

And I think we should add tests for this. e.g., in the ASTImporterTests.cpp.

I'm not sure which test would be useful in this case. We kinda already test that different modules agree which attributes should be ODR hashable. That is enforced by detecting attribute mismatches in odr_hash.cpp (if attributes aren't hashable - no mismatches).

clang/include/clang/Basic/Attr.td
552–553

At this point I don't know what the default value should be, for that I'll need to see how many attributes should be hashable. This change is mostly to prove the direction is viable and reasonable. And TypeAttr, DeclOrTypeAttr is a good starting point to decide which attributes should be hashed.

Good idea with expanding the comment. Though I need some time to prepare non-abhorrent explanation (gosh, docs are harder than code).

clang/lib/AST/AttrImpl.cpp
16

Yep. AttrImpl.cpp includes "clang/AST/AttrImpl.inc" where, for example, we have

void AlignValueAttr::processODRHash(
    llvm::FoldingSetNodeID &ID, ODRHash &Hash) const {
  ID.AddInteger(Attr::getKind());
  const Expr *AlignmentExpr = getAlignment();
  ID.AddBoolean(AlignmentExpr);
  if (AlignmentExpr)
    Hash.AddStmt(AlignmentExpr);
}

and we need the full declaration of ODRHash to call ODRHash::AddStmt.

242

AttrImpl.inc include I've mentioned.

And I think we should add tests for this. e.g., in the ASTImporterTests.cpp.

I'm not sure which test would be useful in this case. We kinda already test that different modules agree which attributes should be ODR hashable. That is enforced by detecting attribute mismatches in odr_hash.cpp (if attributes aren't hashable - no mismatches).

Yeah, I admit it is redundant from some perspective. I think the lit tests are different with the unittest. The lit tests check if the end behavior is expected. And the unittest tests checks if the status of the attribute is expected. For example, it is possible to discard the changes in this patch and keep these lit tests passes.

I think more tests are always good. Especially now it is not hard to add these tests.

aaron.ballman added inline comments.Nov 30 2022, 4:39 AM
clang/include/clang/Basic/Attr.td
552–553

Okay, I'm fine being conservative until we figure out what the right default is.