[PATCH] D110422: [AIX] Enable PGO without LTO

MaskRay added a comment. I still think the description can be rephrased to be shorter and clearer, and am not very sure this workaround should be added. But I will take a vacation and be back after one week and don't want to appear that I am blocking this change. So LGTM once you decrease the n

[PATCH] D110422: [AIX] Enable PGO without LTO

jsji updated this revision to Diff 375688. jsji marked an inline comment as done. jsji added a comment. Address comments. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D110422/new/ https://reviews.llvm.org/D110422 Files: clang/lib/Driver/ToolChai

[PATCH] D110422: [AIX] Enable PGO without LTO

MaskRay added inline comments. Comment at: llvm/lib/Transforms/Instrumentation/InstrProfiling.cpp:865 + // Due to the current limitation of binder, the duplicate weak symbols in the + // same csect won't be discarded. When there are duplicate weak symbols, "D

[PATCH] D110422: [AIX] Enable PGO without LTO

jsji added a comment. > "as binder can NOT discard a subset of a csect."? Thanks, this does looks better! > The linker (binder) doesn't have to discard a subset of csect, but it should > ensure there are not two non-local symbols with the same name. > The latter is what I request. > Without

[PATCH] D110422: [AIX] Enable PGO without LTO

MaskRay added a comment. In D110422#3025911 , @jsji wrote: >> "as binder can NOT discard only part of a csect." this part caused confusion >> to me... > > OK, yeah, csect is special to XCOFF, so this is not the same concept of > sections in ELF. "as bi

[PATCH] D110422: [AIX] Enable PGO without LTO

jsji added a comment. > "as binder can NOT discard only part of a csect." this part caused confusion > to me... OK, yeah, csect is special to XCOFF, so this is not the same concept of sections in ELF. > I haven't verified but it is possible that PGO picks the first pair (zero > value) and hav

[PATCH] D110422: [AIX] Enable PGO without LTO

MaskRay added a comment. > However, on AIX, the current binder can NOT discard the weak symbols if we > put all of them into the same csect, as binder can NOT discard only part of a > csect. "as binder can NOT discard only part of a csect." this part caused confusion to me... > CountersOffset

[PATCH] D110422: [AIX] Enable PGO without LTO

jsji added a comment. OK, I may not describe the example clearly. Let me use example code with offsets and llvm internal calculations as the example, so that you might be clearer. Let us say we have 2 objects , which both have weak function foo (and some non-weak functions). [2054] m 0x1

[PATCH] D110422: [AIX] Enable PGO without LTO

MaskRay added a comment. Sorry, I still don't understand this response: > We have 3 sets weak symbols here: weak_func, profc_weak_foo, profd_weak_func. > We can't ensure that binder always choose 3 of them from same object. So this part of the description isn't clear to me: > However, on AIX,

[PATCH] D110422: [AIX] Enable PGO without LTO

jsji added a comment. In D110422#3025133 , @MaskRay wrote: > The description isn't clear why the previous `weak` symbol usage does not > work. Have you verified that it does not work? Yes, we started with the `weak` symbols for `profc`/`profd` first, an

[PATCH] D110422: [AIX] Enable PGO without LTO

MaskRay added a comment. The description isn't clear why the previous `weak` symbol usage does not work. Have you verified that it does not work? Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D110422/new/ https://reviews.llvm.org/D110422 _

[PATCH] D110422: [AIX] Enable PGO without LTO

jsji added a comment. @MaskRay Do you have further comments or alternative solutions? Thanks. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D110422/new/ https://reviews.llvm.org/D110422 ___ cfe-commits ma

[PATCH] D110422: [AIX] Enable PGO without LTO

jsji added a comment. Using to `PrivateLinkage`, all the profc/profd for each weak symbol will be *local* to objects, and all kept in the csect, so we won't have problem. The downside is that we won't be able to discard the duplicated counters and profile data, but those can not be discarded ev

[PATCH] D110422: [AIX] Enable PGO without LTO

jsji added a comment. We have verified all the profile counters in SPEC , all are OK. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D110422/new/ https://reviews.llvm.org/D110422 ___ cfe-commits mailing li

[PATCH] D110422: [AIX] Enable PGO without LTO

MaskRay added a comment. In D110422#3021572 , @jsji wrote: > In D110422#3021535 , @MaskRay wrote: > >> The description is still unclear. >> >> Say a.o has a weak `foo, __profc_foo, __profd_foo`, b.o has a weak `fo

[PATCH] D110422: [AIX] Enable PGO without LTO

jsji added a comment. In D110422#3021535 , @MaskRay wrote: > The description is still unclear. > > Say a.o has a weak `foo, __profc_foo, __profd_foo`, b.o has a weak `foo, > __profc_foo, __profd_foo`. > The linker picks the definitions from `a.o`. In the

[PATCH] D110422: [AIX] Enable PGO without LTO

MaskRay added a comment. The description is still unclear. Say a.o has a weak `foo, __profc_foo, __profd_foo`, b.o has a weak `foo, __profc_foo, __profd_foo`. The linker picks the definitions from `a.o`. In the PGO implementation, it doesn't whether the non-discarded b.o `__profd_foo` has garba

[PATCH] D110422: [AIX] Enable PGO without LTO

jsji added a comment. Pos_Rel __profc_weak_func notation is just the normal relocation to symbol __profc_weak_func. The relocation here is to calculate the relative pointer (offset) to __profc__weak_func from __profd__weak__func. In example above, the offsets value after binding will be the sa

[PATCH] D110422: [AIX] Enable PGO without LTO

MaskRay added a comment. In D110422#3021283 , @jsji wrote: >> In other binary formats the first weak definition is selected and other weak >> definitions are discarded. >> Do you mean that AIX ld doesn't pick the first weak definition? > > No. I think th

[PATCH] D110422: [AIX] Enable PGO without LTO

jsji added a comment. > In other binary formats the first weak definition is selected and other weak > definitions are discarded. > Do you mean that AIX ld doesn't pick the first weak definition? No. I think this is exactly what is causing problem here. eg: if we have two weak symbols （weak_fun

[PATCH] D110422: [AIX] Enable PGO without LTO

MaskRay added a comment. > we can NOT guarantee that the relocations get resolved to the intended weak > symbol, so we can not ensure the correctness of the relative CounterPtr, so > we have to use private linkage for counter and data symbols. In other binary formats the first weak definition i

[PATCH] D110422: [AIX] Enable PGO without LTO

jsji added inline comments. Comment at: llvm/lib/Transforms/Instrumentation/InstrProfiling.cpp:869 + // symbol, so we can not ensure the correctness of the relative CounterPtr, so + // we have to use private linkage for counter and data symbols. + if (TT.isOSBinFormatXCOFF())

[PATCH] D110422: [AIX] Enable PGO without LTO

jsji added inline comments. Comment at: llvm/lib/Transforms/Instrumentation/InstrProfiling.cpp:869 + // symbol, so we can not ensure the correctness of the relative CounterPtr, so + // we have to use private linkage for counter and data symbols. + if (TT.isOSBinFormatXCOFF())

[PATCH] D110422: [AIX] Enable PGO without LTO

jsji updated this revision to Diff 374879. jsji added a comment. Address comments. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D110422/new/ https://reviews.llvm.org/D110422 Files: clang/lib/Driver/ToolChains/AIX.cpp clang/lib/Driver/ToolChain

[PATCH] D110422: [AIX] Enable PGO without LTO

MaskRay added inline comments. Comment at: llvm/lib/Transforms/Instrumentation/InstrProfiling.cpp:978 if (NS == 0 && !(DataReferencedByCode && NeedComdat && !Renamed) && - (TT.isOSBinFormatELF() || + (TT.isOSBinFormatELF() || TT.isOSBinFormatXCOFF() || (!Data

[PATCH] D110422: [AIX] Enable PGO without LTO

MaskRay requested changes to this revision. MaskRay added inline comments. This revision now requires changes to proceed. Comment at: llvm/lib/Transforms/Instrumentation/InstrProfiling.cpp:869 + // symbol, so we can not ensure the correctness of the relative CounterPtr, so + /

[PATCH] D110422: [AIX] Enable PGO without LTO

jsji updated this revision to Diff 374867. jsji added a comment. Restore the limitation of sample profiling. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D110422/new/ https://reviews.llvm.org/D110422 Files: clang/lib/Driver/ToolChains/AIX.cpp

[PATCH] D110422: [AIX] Enable PGO without LTO

jsji created this revision. jsji added reviewers: PowerPC, Whitney, w2yehia, MaskRay. Herald added subscribers: wenlei, hiraditya, inglorion. jsji requested review of this revision. Herald added projects: clang, LLVM. Herald added subscribers: llvm-commits, cfe-commits. On AIX, we relied on LTO to