This is an archive of the discontinued LLVM Phabricator instance.

[clang][objc][codegen] Skip emitting ObjC category metadata when the category is empty
ClosedPublic

Authored by guitard0g on Nov 8 2021, 7:56 PM.

Details

Summary

Currently, if we create a category in ObjC that is empty, we still emit runtime metadata for that category. This is a scenario that could commonly be run into when using __attribute__((objc_direct_members)), which elides the need for much of the category metadata. This is slightly wasteful and can be easily skipped by checking the category metadata contents during CodeGen.

rdar://66177182

Diff Detail

Event Timeline

guitard0g requested review of this revision.Nov 8 2021, 7:56 PM
guitard0g created this revision.
Herald added a project: Restricted Project. · View Herald TranscriptNov 8 2021, 7:56 PM
Herald added a subscriber: cfe-commits. · View Herald Transcript
guitard0g edited the summary of this revision. (Show Details)Nov 8 2021, 7:59 PM
guitard0g edited the summary of this revision. (Show Details)
guitard0g updated this revision to Diff 385870.Nov 9 2021, 10:02 AM

Fixed code formatting.

zoecarver accepted this revision.Nov 11 2021, 2:12 PM
zoecarver added inline comments.
clang/lib/CodeGen/CGObjCMac.cpp
6681

Does this really need to be an &=? Is there any case where isEmptyCategory isn't true here?

clang/test/CodeGenObjC/category-class-empty.m
2

Is -O3 needed here?

clang/test/CodeGenObjC/non-lazy-classes.m
45–48

This is to prevent another test from failing? Makes sense.

This revision is now accepted and ready to land.Nov 11 2021, 2:12 PM
jkorous accepted this revision.Nov 11 2021, 3:29 PM

LGTM if we have test coverage for all the cases.

clang/test/CodeGenObjC/category-class-empty.m
2

I would even think -O0 is better and consider also -disable-llvm-passes. Ultimately the goal is to make sure frontend codegen doesn't emit the metadata so the less processing of the IR in the backend the more sensitive the test will be.

11

I assume all the cases when we want to emit the metadata (at least one instance method, at least one class method, ...) are covered by other tests, right?
If there's a case that's missing we should add a test for it.

ahatanak added inline comments.Nov 11 2021, 10:52 PM
clang/lib/CodeGen/CGObjCMac.cpp
6712

Is it not possible to check whether the category is empty here? If it's empty, you can avoid creating the global variable and abandon the builder.

guitard0g updated this revision to Diff 386943.Nov 12 2021, 1:44 PM

Address comments and fix fragile test to use -O0 and -disable-llvm-passes.

guitard0g updated this revision to Diff 386947.Nov 12 2021, 1:51 PM

Abandon builder earlier before creating global variable.

guitard0g marked 6 inline comments as done.Nov 12 2021, 1:53 PM
guitard0g added inline comments.
clang/lib/CodeGen/CGObjCMac.cpp
6681

Good point, fixed.

6712

Good point, done.

clang/test/CodeGenObjC/category-class-empty.m
2

Good catch, changed to -O0 with -disable-llvm-passes.

11

Yeah a fair number of tests would break if we started removing categories with anything inside of them.

clang/test/CodeGenObjC/non-lazy-classes.m
45–48

Yeah this is the one case where the IRGen check was looking for the category even though it's empty. Should be a harmless change.

guitard0g updated this revision to Diff 386949.Nov 12 2021, 1:54 PM
guitard0g marked 5 inline comments as done.

Nit: deleted empty line.

@ahatanak I don't have commit access, any chance you could commit this for me?