This is an archive of the discontinued LLVM Phabricator instance.

[InstrProfiling] Support relative CountersPtr for PlatformOther
ClosedPublic

Authored by jsji on Aug 18 2021, 8:06 AM.

Details

Summary

D104556 change the CountersPtr to be relative, however, it did not
update the pointer initialization in llvm_profile_register_function,
so the platform (eg:AIX) that use
llvm_profile_register_function is now totaly
broken, any PGO code will SEGV.

This patch update the code to reflect that the Data->CountersPtr is now
relative.

Diff Detail

Event Timeline

jsji created this revision.Aug 18 2021, 8:06 AM
jsji requested review of this revision.Aug 18 2021, 8:06 AM
Herald added a project: Restricted Project. · View Herald TranscriptAug 18 2021, 8:06 AM
Herald added a subscriber: Restricted Project. · View Herald Transcript
jsji added a comment.EditedAug 18 2021, 8:09 AM

Unfortunately, looks like we haven't enabled any of the test in compiler-rt/test/profile for AIX, as it allows the host_os in whilelist only, which was not noticed before.
We will work on enable those tests in AIX, probably in another patch.

Appreciate if there is some specific lit test can be added to test this behavior, I can add that first. Thanks.

Can you modify lit.cfg.py file to add AIX in supported host platforms and start from there?

jsji added a comment.Aug 18 2021, 9:04 AM

Can you modify lit.cfg.py file to add AIX in supported host platforms and start from there?

Thanks David, yes, that's what I am working on, so far there are only a few of them passing. I plan to work on porting those in following patches. Is that OK?

For this patch, I have verified that it works on our spec build.

hubert.reinterpretcast added inline comments.
compiler-rt/lib/profile/InstrProfilingPlatformOther.c
49

I suggest keeping "base + offset" expressions with the "base" first (in left-to-right order).
Also, should D104556 not have changed the name of the CounterPtr member?

davidxl accepted this revision.Aug 18 2021, 9:19 AM

LGTM -- with Hubert's suggestions addressed.

This revision is now accepted and ready to land.Aug 18 2021, 9:19 AM
MaskRay accepted this revision.Aug 18 2021, 9:22 AM
jsji updated this revision to Diff 367243.Aug 18 2021, 10:12 AM

Address comments.

This revision was landed with ongoing or failed builds.Aug 18 2021, 10:46 AM
This revision was automatically updated to reflect the committed changes.