This is an archive of the discontinued LLVM Phabricator instance.

[clang] Convert a few tests to opaque pointers
ClosedPublic

Authored by barannikov88 on May 18 2023, 10:48 AM.

Diff Detail

Event Timeline

barannikov88 created this revision.May 18 2023, 10:48 AM
Herald added a project: Restricted Project. · View Herald TranscriptMay 18 2023, 10:48 AM
barannikov88 requested review of this revision.May 18 2023, 10:48 AM
Herald added a project: Restricted Project. · View Herald TranscriptMay 18 2023, 10:48 AM
barannikov88 added inline comments.May 18 2023, 10:54 AM
clang/test/CodeGenCXX/const-init-cxx11.cpp
353

This was one suspicious change. An anonymous struct became named.

clang/test/CodeGenCXX/constructor-destructor-return-this.cpp
156

The parameter has changed its name for some reason.

nikic accepted this revision.May 19 2023, 12:41 AM

LGTM

clang/test/CodeGenCXX/const-init-cxx11.cpp
353

Not sure why exactly this happened, but should be harmless. I believe we sometimes generate anon structs for initializaton because the types used for initialization are not always compatible with the nominal LLVM memory type -- I guess there previously was a mismatch in pointer types here or something.

This revision is now accepted and ready to land.May 19 2023, 12:41 AM
barannikov88 added inline comments.May 19 2023, 8:55 AM
clang/test/CodeGenCXX/const-init-cxx11.cpp
353

This is where the behavior diverges:

llvm::Constant *ConstantAggregateBuilder::buildFrom(
    CodeGenModule &CGM, ArrayRef<llvm::Constant *> Elems,
    ArrayRef<CharUnits> Offsets, CharUnits StartOffset, CharUnits Size,
    bool NaturalLayout, llvm::Type *DesiredTy, bool AllowOversized) {
...
  // Pick the type to use.  If the type is layout identical to the desired
  // type then use it, otherwise use whatever the builder produced for us.
  if (llvm::StructType *DesiredSTy = dyn_cast<llvm::StructType>(DesiredTy)) {
    if (DesiredSTy->isLayoutIdentical(STy))
      STy = DesiredSTy;
  }
...

With typed pointers, STy and DesiredSTy respectively are:

{ i8** }
%struct.nsMemoryImpl = type { i32 (...)** }

isLayoutIdentical, despite its name, actually checks for full equivalence of structs, not just the layout. That is, it is no different from operator==.

The code snippet above is the only use of isLayoutIdentical.

Resolve merge conflicts in one test

barannikov88 added inline comments.May 19 2023, 9:20 AM
clang/test/CodeGenCXX/const-init-cxx11.cpp
353

That is, it is no different from operator==.

Quick test shows this is not true...

This revision was landed with ongoing or failed builds.May 19 2023, 12:12 PM
This revision was automatically updated to reflect the committed changes.
nikic added a comment.May 25 2023, 1:04 PM

I wanted to check whether you plan to do more opaque pointer test conversions in clang. Just to make sure we don't duplicate work...

I wanted to check whether you plan to do more opaque pointer test conversions in clang. Just to make sure we don't duplicate work...

I'm done for now