-
Notifications
You must be signed in to change notification settings - Fork 15.2k
[StaticDataLayout] Factor out a helper function for section prefix eligibility and use it in both optimizer and codegen #162348
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
base: users/mingmingl-llvm/module
Are you sure you want to change the base?
Changes from all commits
f68b06f
331888b
dcb9f00
3565bc0
905b80f
95292a6
64be09f
4a90c77
f9c6266
34b47c9
b880ee3
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change | ||||
---|---|---|---|---|---|---|
|
@@ -75,7 +75,7 @@ bool StaticDataAnnotator::runOnModule(Module &M) { | |||||
|
||||||
bool Changed = false; | ||||||
for (auto &GV : M.globals()) { | ||||||
if (GV.isDeclarationForLinker()) | ||||||
if (!llvm::memprof::IsAnnotationOK(GV)) | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This helper function includes more checks than just isDeclarationForLinker - so is this change really NFC? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. yeah I agree. I think this PR is more of 'NFCI' (non functional change intended) or no user-visible change intended. I removed the 'NFC' tag so (unfamiliar) readers won't get (unintentionally) fooled.. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. But are the effects of this change and the other one I mentioned earlier not observable by tests? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Yeah, the test coverage is limited to [1] before. Improved the test coverage now, PTAL, thanks! [1] llvm-project/llvm/test/Transforms/PGOProfile/data-access-profile.ll Lines 28 to 29 in 139a6bf
|
||||||
continue; | ||||||
|
||||||
// The implementation below assumes prior passes don't set section prefixes, | ||||||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This check is new afaict - so is this NFC?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ah sorry, you are correct.
This is from another similar call site [1] and this PR means to cover that one. Updated the PR.
[1]
llvm-project/llvm/lib/CodeGen/StaticDataSplitter.cpp
Lines 133 to 137 in 30b9ef8