This is an archive of the discontinued LLVM Phabricator instance.

[CodeGen] Make element type in emitArrayDestroy() predictable
ClosedPublic

Authored by nikic on Dec 23 2021, 7:15 AM.

Details

Summary

When calling emitArrayDestroy(), the pointer will usually have ConvertTypeForMem(EltType) as the element type, as one would expect. However, globals with initializers sometimes don't use the same types as values normally would, e.g. here the global uses { double, i32 } rather than %struct.T as element type.

Add an early cast to the global destruction path to avoid this special case. The cast would happen lateron anyway, it only gets moved to an earlier point.

Diff Detail

Unit TestsFailed

Event Timeline

nikic requested review of this revision.Dec 23 2021, 7:15 AM
nikic created this revision.
nikic updated this revision to Diff 397018.Jan 3 2022, 12:47 AM

Merge fixes for element type uses into this patch.

rjmccall added inline comments.Jan 3 2022, 9:50 PM
clang/lib/CodeGen/CGDecl.cpp
2257

Hmm. I would expect the caller to be responsible for passing in a correctly-typed pointer here.

clang/lib/CodeGen/CodeGenFunction.cpp
2115 ↗(On Diff #397018)

I think I have a similar question here. I understand that globals don't always have the right type because of initializers, but the code that's emitting the reference to the global generally ought to be casting it to the right type instead of forcing all the downstream code to cast.

nikic updated this revision to Diff 397231.Jan 4 2022, 2:16 AM

Perform cast earlier, in EmitDeclDestroy().

nikic added inline comments.Jan 4 2022, 2:18 AM
clang/lib/CodeGen/CGDecl.cpp
2257

The caller is responsible at this point. This only explicitly computes the type to avoid the getPointerElementType() call.

clang/lib/CodeGen/CodeGenFunction.cpp
2115 ↗(On Diff #397018)

I've moved the cast into EmitDeclDestroy() and left this code alone. Does this look better?

nikic updated this revision to Diff 397281.Jan 4 2022, 6:33 AM

Rebase over D116587 to get rid of the double geps.

rjmccall added inline comments.Jan 4 2022, 11:59 AM
clang/lib/CodeGen/CGDecl.cpp
2257

Okay. Is it excessively annoying to pass in an Address for begin instead of separately passing elementAlign? We'd get the element type for free then.

nikic added inline comments.Jan 4 2022, 12:08 PM
clang/lib/CodeGen/CGDecl.cpp
2257

Yeah, I did give that a try -- accepting Address in emitArrayDestroy() in particular is easy, but once you try to propagate it up the call chain it turns out that there are a lot of non-trivial callers.

nikic retitled this revision from [CodeGen] Make element type in emitArrayLength() predictable to [CodeGen] Make element type in emitArrayDestroy() predictable.Jan 10 2022, 12:17 AM
nikic edited the summary of this revision. (Show Details)
rjmccall accepted this revision.Jan 10 2022, 9:50 PM

Alright. We can do this for now, then.

This revision is now accepted and ready to land.Jan 10 2022, 9:50 PM
This revision was landed with ongoing or failed builds.Jan 11 2022, 12:25 AM
This revision was automatically updated to reflect the committed changes.
Herald added a project: Restricted Project. · View Herald TranscriptJan 11 2022, 12:25 AM
Herald added a subscriber: cfe-commits. · View Herald Transcript