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

[Fangrui Song via Phabricator via cfe-commits]

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 number of `if (TT.isOSBinFormatXCOFF()) {` from 
two to one.
If you track where `Linkage` and `Visibility` are updated it should be clear 
one place is not needed.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D110422/new/

https://reviews.llvm.org/D110422

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Jinsong Ji via Phabricator via cfe-commits]

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/ToolChains/AIX.cpp
  clang/lib/Driver/ToolChains/Clang.cpp
  clang/test/Driver/unsupported-option.c
  clang/test/Profile/cxx-templates.cpp
  llvm/lib/Transforms/Instrumentation/InstrProfiling.cpp
  llvm/test/Instrumentation/InstrProfiling/profiling.ll

Index: llvm/test/Instrumentation/InstrProfiling/profiling.ll
===
--- llvm/test/Instrumentation/InstrProfiling/profiling.ll
+++ llvm/test/Instrumentation/InstrProfiling/profiling.ll
@@ -45,8 +45,8 @@
 ; MACHO: @__profd_foo_weak = weak hidden global
 ; COFF: @__profc_foo_weak = weak hidden global
 ; COFF: @__profd_foo_weak = private global
-; XCOFF: @__profc_foo_weak = weak hidden global
-; XCOFF: @__profd_foo_weak = weak hidden global
+; XCOFF: @__profc_foo_weak = private global
+; XCOFF: @__profd_foo_weak = private global
 define weak void @foo_weak() {
   call void @llvm.instrprof.increment(i8* getelementptr inbounds ([8 x i8], [8 x i8]* @__profn_foo_weak, i32 0, i32 0), i64 0, i32 1, i32 0)
   ret void
@@ -71,8 +71,8 @@
 ; MACHO: @__profd_foo_inline = linkonce_odr hidden global
 ; COFF: @__profc_foo_inline = linkonce_odr hidden global{{.*}} section ".lprfc$M", align 8
 ; COFF: @__profd_foo_inline = private global{{.*}} section ".lprfd$M", align 8
-; XCOFF: @__profc_foo_inline = linkonce_odr hidden global
-; XCOFF: @__profd_foo_inline = linkonce_odr hidden global
+; XCOFF: @__profc_foo_inline = private global
+; XCOFF: @__profd_foo_inline = private global
 define linkonce_odr void @foo_inline() {
   call void @llvm.instrprof.increment(i8* getelementptr inbounds ([10 x i8], [10 x i8]* @__profn_foo_inline, i32 0, i32 0), i64 0, i32 1, i32 0)
   ret void
@@ -84,8 +84,8 @@
 ; MACHO: @__profd_foo_extern = linkonce_odr hidden global
 ; COFF: @__profc_foo_extern = linkonce_odr hidden global {{.*}}section ".lprfc$M", comdat, align 8
 ; COFF: @__profd_foo_extern = private global {{.*}}section ".lprfd$M", comdat($__profc_foo_extern), align 8
-; XCOFF: @__profc_foo_extern = linkonce_odr hidden global
-; XCOFF: @__profd_foo_extern = linkonce_odr hidden global
+; XCOFF: @__profc_foo_extern = private global
+; XCOFF: @__profd_foo_extern = private global
 define available_externally void @foo_extern() {
   call void @llvm.instrprof.increment(i8* getelementptr inbounds ([10 x i8], [10 x i8]* @__profn_foo_extern, i32 0, i32 0), i64 0, i32 1, i32 0)
   ret void
Index: llvm/lib/Transforms/Instrumentation/InstrProfiling.cpp
===
--- llvm/lib/Transforms/Instrumentation/InstrProfiling.cpp
+++ llvm/lib/Transforms/Instrumentation/InstrProfiling.cpp
@@ -862,6 +862,15 @@
   GlobalValue::LinkageTypes Linkage = NamePtr->getLinkage();
   GlobalValue::VisibilityTypes Visibility = NamePtr->getVisibility();
 
+  // Due to the limitation of binder as of 2021/09/28, the duplicate weak
+  // symbols in the same csect won't be discarded. When there are duplicate weak
+  // symbols, 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.
+  if (TT.isOSBinFormatXCOFF()) {
+Linkage = GlobalValue::PrivateLinkage;
+Visibility = GlobalValue::DefaultVisibility;
+  }
   // Move the name variable to the right section. Place them in a COMDAT group
   // if the associated function is a COMDAT. This will make sure that only one
   // copy of counters of the COMDAT function will be emitted after linking. Keep
@@ -971,6 +980,17 @@
 Linkage = GlobalValue::PrivateLinkage;
 Visibility = GlobalValue::DefaultVisibility;
   }
+
+  // Due to the limitation of binder as of 2021/09/28, the duplicate weak
+  // symbols in the same csect won't be discarded. When there are duplicate weak
+  // symbols, 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.
+  if (TT.isOSBinFormatXCOFF()) {
+Linkage = GlobalValue::PrivateLinkage;
+Visibility = GlobalValue::DefaultVisibility;
+  }
+
   auto *Data =
   new GlobalVariable(*M, DataTy, false, Linkage, nullptr, DataVarName);
   // Reference the counter variable with a label difference (link-time
Index: clang/test/Profile/cxx-templates.cpp
===
--- clang/test/Profile/cxx-templates.cpp
+++ clang/test/Profile/cxx-templates.cpp
@@ -10,8 +10,10 @@
 // RUN: FileCheck --input-file=%tuse -check-prefix=T0USE -check-prefix=ALL %s
 // RUN: FileCheck 

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

[Fangrui Song via Phabricator via cfe-commits]

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,

"Due to the current limitation of binder,"

Replace "current" with "as of version XX or date"


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D110422/new/

https://reviews.llvm.org/D110422

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Jinsong Ji via Phabricator via cfe-commits]

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 section deduplication we just end up with PDP-11 a.out extended with 
> .weak directive, it isn't too bad. Mach-O uses this model as well.
> (PE-COFF/ELF properly deduplicates in the absence of section based GC.)
> Having two non-local symbols could cause serious symbol resolution problems.

That I can definitely forward to AIX binder team, but we can't expect they 
implement it right away for us.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D110422/new/

https://reviews.llvm.org/D110422

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Fangrui Song via Phabricator via cfe-commits]

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 binder can NOT discard a subset of a csect."?

>> I haven't verified but it is possible that PGO picks the first pair (zero 
>> value) and have an incorrect counter for `foo`.
>
> As I mentioned before, we verified (with `llvm-profdata show --all-functions 
> --counts`) on all SPEC benchmarks that all the counters are good with private 
> linkage.
>
>> ---
>>
>> Does your example imply that the weak symbol `__profc_foo` has 2 definitions 
>> with conflicting values?
>
> No, they are identical weak functions.  Those offsets are actually from real 
> example using `std::stringbuf`, 
> the foo is actually 
> `_ZNSt3__115basic_stringbufIcNS_11char_traitsIcEENS_9allocatorIcEEEC2Ej`.
>
>> Any idea whether the linker bug will be fixed?
>
> Unfortunately, I just double confirmed with AIX linker (binder) owner, 
> there is NO plan in near future of lifting the limitation that binder can NOT 
> discard only part of a csect.

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 section deduplication we just end up with PDP-11 a.out extended with 
.weak directive, it isn't too bad.
Having two non-local symbols could cause serious symbol resolution problems.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D110422/new/

https://reviews.llvm.org/D110422

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Jinsong Ji via Phabricator via cfe-commits]

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 have an incorrect counter for `foo`.

As I mentioned before, we verified (with `llvm-profdata show --all-functions 
--counts`) that all the counters are good.

> ---
>
> Does your example imply that the weak symbol `__profc_foo` has 2 definitions 
> with conflicting values?

No, they are identical weak functions.  Those offsets are actually from real 
example using `std::stringbuf`, 
the foo is actually 
`_ZNSt3__115basic_stringbufIcNS_11char_traitsIcEENS_9allocatorIcEEEC2Ej`.

> Any idea whether the linker bug will be fixed?

Unfortunately, I just double confirmed with AIX linker (binder) owner, 
there is NO plan of lifting the limitation that binder can NOT discard only 
part of a csect.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D110422/new/

https://reviews.llvm.org/D110422

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Fangrui Song via Phabricator via cfe-commits]

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 is now 0x1da3 ( CounterPtr - CountersDelta / 
> sizeof(unit64_6))
>
> CountersOffset > MaxNumCounters !

It is true that there are two pairs of weak profc/profd, but the first profc 
has a zero value, so I assume that it is fine?

The second pair of profc/profd has correct value.

I haven't verified but it is possible that PGO picks the first pair (zero 
value) and have an incorrect counter for `foo`.

---

Does your example imply that the weak symbol `__profc_foo` has 2 definitions 
with conflicting values? 
I presume it can cause more fallout.
Any idea whether the linker bug will be fixed?


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D110422/new/

https://reviews.llvm.org/D110422

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Jinsong Ji via Phabricator via cfe-commits]

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   0x11ec0 .data 1  unamex
__llvm_prf_cnts
  [2056]  m   0x11ef8 .data 1weak
__profc__foo
  [1389]  m   0x110001540 .data 1  unamex
__llvm_prf_cnts
  [1391]  m   0x1100015a8 .data 1weak
__profc__foo<== chosen by binder
  
  [2290]  m   0x110001ca8 .data 1  unamex
__llvm_prf_data
  [2292]  m   0x110001ce0 .data 1weak
__profd__foo
  [1633]  m   0x110003678 .data 1  unamex
__llvm_prf_data
  [1635]  m   0x1100036b0 .data 1weak
__profd__foo<===  chosen by binder

In binding, binder chose 0x1100015a8 for `__profc__foo`, and 0x1100036b0 for 
`__profd__foo`. 
(Not the 1st in the csect, but 1st seen by binder)

At the beginning `CountersDelta` is 0xf218. ( 0x11ec0 [2054]  - 
0x110001ca8 [2290] ) .

The first record is for non-weak function, so we are OK.

Then we move forward to read foo counters, 
`CountersDelta` is then updated to 0xf1e0 ( 0xf218 - 
sizeof(Data))
`CounterPtr` is now 0x1100015a8 - 0x1100036b0 = 0xdef8

CountersOffset is now 0x1da3  ( CounterPtr - CountersDelta / 
sizeof(unit64_6))

CountersOffset > MaxNumCounters !

If we have more weak symbols, the symbols chosen by binder may be  interleaving 
in the csects,
we won't be able to calculate the CountersOffset correctly for all of them.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D110422/new/

https://reviews.llvm.org/D110422

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Fangrui Song via Phabricator via cfe-commits]

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, 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.
>
> This creates a unique challenge for using those symbols to calculate some 
> relative offset.

Say a.o has a weak `foo, __profc_foo, __profd_foo`. The `__profd_foo` 
references the `__profc_foo`

b.o has another set of weak `foo, __profc_foo, __profd_foo`. The `__profd_foo` 
references the `__profc_foo`.

What's the linker (called binder?) behavior? "We can't ensure that binder 
always choose 3 of them from same object."?
But is that a problem? For linkonce_odr functions, they should have identical 
semantics in a.o and b.o.
(There can be ODR issues if you use advanced PGO techniques like value 
profiling, but I assume that it does not work and will be difficult to make 
work and more importantly, it is also of lower value anyway.)

So the linker picking the a.o `foo` and the b.o `__profc_foo/__profd_foo` isn't 
a poblem.

I understand that PrivateLinkage can avoid the issues but you need to justify 
the complexity in `InstrProfiling.cpp` with the value.
Currently I don't think using weak symbol is a big problem, especially if the 
AIX linker will be fixed anyway in the future.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D110422/new/

https://reviews.llvm.org/D110422

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Jinsong Ji via Phabricator via cfe-commits]

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, and tested 
it. We met problems, and we spent time investigate why they are not working.

I have update the description to include most of the details. Let me know if it 
is still unclear.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D110422/new/

https://reviews.llvm.org/D110422

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Fangrui Song via Phabricator via cfe-commits]

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

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Jinsong Ji via Phabricator via cfe-commits]

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 mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Jinsong Ji via Phabricator via cfe-commits]

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 even if we keep the weak linkage,
due to the binder limitation of not discarding only part of the csect either .


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D110422/new/

https://reviews.llvm.org/D110422

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Jinsong Ji via Phabricator via cfe-commits]

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 list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Fangrui Song via Phabricator via cfe-commits]

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 `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 garbage value.
>
> 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.

Oh, this is a serious enough bug. I am not sure working around the bug by 
switching to `PrivateLinkage` will help.
The values will be wrong as well.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D110422/new/

https://reviews.llvm.org/D110422

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Jinsong Ji via Phabricator via cfe-commits]

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 PGO implementation, it 
> doesn't whether the non-discarded b.o `__profd_foo` has garbage value.

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.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D110422/new/

https://reviews.llvm.org/D110422

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Fangrui Song via Phabricator via cfe-commits]

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 garbage value.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D110422/new/

https://reviews.llvm.org/D110422

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Jinsong Ji via Phabricator via cfe-commits]

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 same for both 
instances, as the symbol __profc__weak__func will be the one that is chosen by 
binder. Let us say it is the value of first instances.

Then when we calculate the profc_weak_func pointer for 2nd instances, we get 
garbage value.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D110422/new/

https://reviews.llvm.org/D110422

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Fangrui Song via Phabricator via cfe-commits]

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 this is exactly what is causing problem here.
>
> eg: if we have two weak symbols （weak_func）, and we generate the weak 
> counter/data as usual:
>
>   __llvm_prf_cnts:
>   
>   __profc_weak_func(1):
>   
>   __profc_weak_func(2):
>   
>   __llvm_prf_data:
>   
>   __profd_weak func(1):
>   
>   Pos_Rel __profc_weak_func  <=== Relocation to calculate the relative pointer
>   
>   __profd_weak_func(2):
>   
>   Pos_Rel __profc_weak_func  <=== Relocation to calculate the relative pointer
>
> The relative pointer calculation in one of the __profd_weak_fun will be wrong 
> after linking.

Can you elaborate the `Pos_Rel __profc_weak_func` notation?

Note: In other binary formats' scheme, it doesn't matter whether non-first weak 
definitions are discarded or not.
After linking, the non-first weak definitions will be unreachable anyway, so 
their internal references don't really matter.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D110422/new/

https://reviews.llvm.org/D110422

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Jinsong Ji via Phabricator via cfe-commits]

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_func）, and we generate the weak 
counter/data as usual:

  __llvm_prf_cnts:
  
  __profc_weak_func(1):
  
  __profc_weak_func(2):
  
  __llvm_prf_data:
  
  __profd_weak func(1):
  
  Pos_Rel __profc_weak_func  <=== Relocation to calculate the relative pointer
  
  __profd_weak_func(2):
  
  Pos_Rel __profc_weak_func  <=== Relocation to calculate the relative pointer

The relative pointer calculation in __profd_weak_fun will be wrong after 
linking.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D110422/new/

https://reviews.llvm.org/D110422

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Fangrui Song via Phabricator via cfe-commits]

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 is selected and other weak 
definitions are discarded.
Do you mean that AIX ld doesn't pick the first weak definition?


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D110422/new/

https://reviews.llvm.org/D110422

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Jinsong Ji via Phabricator via cfe-commits]

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()) {

jsji wrote:
> MaskRay wrote:
> > Then you can just keep the existing weak symbols.
> > 
> > The weak symbols will not be selected due to linker semantics.
> Not sure what you meant? 
To be clear, keeping the weak linkage is causing issues in our test, and we 
investigate the problem , together with AIX binder owner, and it is the 
limitation in binder support that we can't ensure the relocation getting 
resolved to the intended weak symbol. So this is what we can do right now for 
enabling PGO.

We may be able to lift it this limitation later if binder get updated and do 
what we want, but this is limitation for now.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D110422/new/

https://reviews.llvm.org/D110422

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Jinsong Ji via Phabricator via cfe-commits]

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()) {

MaskRay wrote:
> Then you can just keep the existing weak symbols.
> 
> The weak symbols will not be selected due to linker semantics.
Not sure what you meant? 


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D110422/new/

https://reviews.llvm.org/D110422

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Jinsong Ji via Phabricator via cfe-commits]

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/ToolChains/Clang.cpp
  clang/test/Driver/unsupported-option.c
  clang/test/Profile/cxx-templates.cpp
  llvm/lib/Transforms/Instrumentation/InstrProfiling.cpp
  llvm/test/Instrumentation/InstrProfiling/profiling.ll

Index: llvm/test/Instrumentation/InstrProfiling/profiling.ll
===
--- llvm/test/Instrumentation/InstrProfiling/profiling.ll
+++ llvm/test/Instrumentation/InstrProfiling/profiling.ll
@@ -45,8 +45,8 @@
 ; MACHO: @__profd_foo_weak = weak hidden global
 ; COFF: @__profc_foo_weak = weak hidden global
 ; COFF: @__profd_foo_weak = private global
-; XCOFF: @__profc_foo_weak = weak hidden global
-; XCOFF: @__profd_foo_weak = weak hidden global
+; XCOFF: @__profc_foo_weak = private global
+; XCOFF: @__profd_foo_weak = private global
 define weak void @foo_weak() {
   call void @llvm.instrprof.increment(i8* getelementptr inbounds ([8 x i8], [8 x i8]* @__profn_foo_weak, i32 0, i32 0), i64 0, i32 1, i32 0)
   ret void
@@ -71,8 +71,8 @@
 ; MACHO: @__profd_foo_inline = linkonce_odr hidden global
 ; COFF: @__profc_foo_inline = linkonce_odr hidden global{{.*}} section ".lprfc$M", align 8
 ; COFF: @__profd_foo_inline = private global{{.*}} section ".lprfd$M", align 8
-; XCOFF: @__profc_foo_inline = linkonce_odr hidden global
-; XCOFF: @__profd_foo_inline = linkonce_odr hidden global
+; XCOFF: @__profc_foo_inline = private global
+; XCOFF: @__profd_foo_inline = private global
 define linkonce_odr void @foo_inline() {
   call void @llvm.instrprof.increment(i8* getelementptr inbounds ([10 x i8], [10 x i8]* @__profn_foo_inline, i32 0, i32 0), i64 0, i32 1, i32 0)
   ret void
@@ -84,8 +84,8 @@
 ; MACHO: @__profd_foo_extern = linkonce_odr hidden global
 ; COFF: @__profc_foo_extern = linkonce_odr hidden global {{.*}}section ".lprfc$M", comdat, align 8
 ; COFF: @__profd_foo_extern = private global {{.*}}section ".lprfd$M", comdat($__profc_foo_extern), align 8
-; XCOFF: @__profc_foo_extern = linkonce_odr hidden global
-; XCOFF: @__profd_foo_extern = linkonce_odr hidden global
+; XCOFF: @__profc_foo_extern = private global
+; XCOFF: @__profd_foo_extern = private global
 define available_externally void @foo_extern() {
   call void @llvm.instrprof.increment(i8* getelementptr inbounds ([10 x i8], [10 x i8]* @__profn_foo_extern, i32 0, i32 0), i64 0, i32 1, i32 0)
   ret void
Index: llvm/lib/Transforms/Instrumentation/InstrProfiling.cpp
===
--- llvm/lib/Transforms/Instrumentation/InstrProfiling.cpp
+++ llvm/lib/Transforms/Instrumentation/InstrProfiling.cpp
@@ -862,6 +862,15 @@
   GlobalValue::LinkageTypes Linkage = NamePtr->getLinkage();
   GlobalValue::VisibilityTypes Visibility = NamePtr->getVisibility();
 
+  // 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,
+  // 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.
+  if (TT.isOSBinFormatXCOFF()) {
+Linkage = GlobalValue::PrivateLinkage;
+Visibility = GlobalValue::DefaultVisibility;
+  }
   // Move the name variable to the right section. Place them in a COMDAT group
   // if the associated function is a COMDAT. This will make sure that only one
   // copy of counters of the COMDAT function will be emitted after linking. Keep
@@ -971,6 +980,17 @@
 Linkage = GlobalValue::PrivateLinkage;
 Visibility = GlobalValue::DefaultVisibility;
   }
+
+  // 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,
+  // 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.
+  if (TT.isOSBinFormatXCOFF()) {
+Linkage = GlobalValue::PrivateLinkage;
+Visibility = GlobalValue::DefaultVisibility;
+  }
+
   auto *Data =
   new GlobalVariable(*M, DataTy, false, Linkage, nullptr, DataVarName);
   // Reference the counter variable with a label difference (link-time
Index: clang/test/Profile/cxx-templates.cpp
===
--- clang/test/Profile/cxx-templates.cpp
+++ clang/test/Profile/cxx-templates.cpp
@@ -10,8 +10,10 @@
 // RUN: FileCheck --input-file=%tuse -check-prefix=T0USE -check-prefix=ALL %s
 // RUN: FileCheck --input-file=%tuse -check-prefix=T100USE -check-prefix=ALL 

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

[Fangrui Song via Phabricator via cfe-commits]

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() ||
(!DataReferencedByCode && TT.isOSBinFormatCOFF( {

This place is for binary formats which can properly GC metadata sections. If 
XCOFF can't, it shouldn't be added here.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D110422/new/

https://reviews.llvm.org/D110422

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Fangrui Song via Phabricator via cfe-commits]

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
+  // we have to use private linkage for counter and data symbols.
+  if (TT.isOSBinFormatXCOFF()) {

Then you can just keep the existing weak symbols.

The weak symbols will not be selected due to linker semantics.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D110422/new/

https://reviews.llvm.org/D110422

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Jinsong Ji via Phabricator via cfe-commits]

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
  clang/lib/Driver/ToolChains/Clang.cpp
  clang/test/Driver/unsupported-option.c
  clang/test/Profile/cxx-templates.cpp
  llvm/lib/Transforms/Instrumentation/InstrProfiling.cpp
  llvm/test/Instrumentation/InstrProfiling/profiling.ll

Index: llvm/test/Instrumentation/InstrProfiling/profiling.ll
===
--- llvm/test/Instrumentation/InstrProfiling/profiling.ll
+++ llvm/test/Instrumentation/InstrProfiling/profiling.ll
@@ -45,8 +45,8 @@
 ; MACHO: @__profd_foo_weak = weak hidden global
 ; COFF: @__profc_foo_weak = weak hidden global
 ; COFF: @__profd_foo_weak = private global
-; XCOFF: @__profc_foo_weak = weak hidden global
-; XCOFF: @__profd_foo_weak = weak hidden global
+; XCOFF: @__profc_foo_weak = private global
+; XCOFF: @__profd_foo_weak = private global
 define weak void @foo_weak() {
   call void @llvm.instrprof.increment(i8* getelementptr inbounds ([8 x i8], [8 x i8]* @__profn_foo_weak, i32 0, i32 0), i64 0, i32 1, i32 0)
   ret void
@@ -71,8 +71,8 @@
 ; MACHO: @__profd_foo_inline = linkonce_odr hidden global
 ; COFF: @__profc_foo_inline = linkonce_odr hidden global{{.*}} section ".lprfc$M", align 8
 ; COFF: @__profd_foo_inline = private global{{.*}} section ".lprfd$M", align 8
-; XCOFF: @__profc_foo_inline = linkonce_odr hidden global
-; XCOFF: @__profd_foo_inline = linkonce_odr hidden global
+; XCOFF: @__profc_foo_inline = private global
+; XCOFF: @__profd_foo_inline = private global
 define linkonce_odr void @foo_inline() {
   call void @llvm.instrprof.increment(i8* getelementptr inbounds ([10 x i8], [10 x i8]* @__profn_foo_inline, i32 0, i32 0), i64 0, i32 1, i32 0)
   ret void
@@ -84,8 +84,8 @@
 ; MACHO: @__profd_foo_extern = linkonce_odr hidden global
 ; COFF: @__profc_foo_extern = linkonce_odr hidden global {{.*}}section ".lprfc$M", comdat, align 8
 ; COFF: @__profd_foo_extern = private global {{.*}}section ".lprfd$M", comdat($__profc_foo_extern), align 8
-; XCOFF: @__profc_foo_extern = linkonce_odr hidden global
-; XCOFF: @__profd_foo_extern = linkonce_odr hidden global
+; XCOFF: @__profc_foo_extern = private global
+; XCOFF: @__profd_foo_extern = private global
 define available_externally void @foo_extern() {
   call void @llvm.instrprof.increment(i8* getelementptr inbounds ([10 x i8], [10 x i8]* @__profn_foo_extern, i32 0, i32 0), i64 0, i32 1, i32 0)
   ret void
Index: llvm/lib/Transforms/Instrumentation/InstrProfiling.cpp
===
--- llvm/lib/Transforms/Instrumentation/InstrProfiling.cpp
+++ llvm/lib/Transforms/Instrumentation/InstrProfiling.cpp
@@ -862,6 +862,15 @@
   GlobalValue::LinkageTypes Linkage = NamePtr->getLinkage();
   GlobalValue::VisibilityTypes Visibility = NamePtr->getVisibility();
 
+  // 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,
+  // 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.
+  if (TT.isOSBinFormatXCOFF()) {
+Linkage = GlobalValue::PrivateLinkage;
+Visibility = GlobalValue::DefaultVisibility;
+  }
   // Move the name variable to the right section. Place them in a COMDAT group
   // if the associated function is a COMDAT. This will make sure that only one
   // copy of counters of the COMDAT function will be emitted after linking. Keep
@@ -966,7 +975,7 @@
   // that other copies must have the same CFG and cannot have value profiling.
   // If no hash suffix, other profd copies may be referenced by code.
   if (NS == 0 && !(DataReferencedByCode && NeedComdat && !Renamed) &&
-  (TT.isOSBinFormatELF() ||
+  (TT.isOSBinFormatELF() || TT.isOSBinFormatXCOFF() ||
(!DataReferencedByCode && TT.isOSBinFormatCOFF( {
 Linkage = GlobalValue::PrivateLinkage;
 Visibility = GlobalValue::DefaultVisibility;
Index: clang/test/Profile/cxx-templates.cpp
===
--- clang/test/Profile/cxx-templates.cpp
+++ clang/test/Profile/cxx-templates.cpp
@@ -10,8 +10,10 @@
 // RUN: FileCheck --input-file=%tuse -check-prefix=T0USE -check-prefix=ALL %s
 // RUN: FileCheck --input-file=%tuse -check-prefix=T100USE -check-prefix=ALL %s
 
-// T0GEN: @[[T0C:__profc__Z4loopILj0EEvv]] = linkonce_odr {{(hidden|dso_local)}} global [2 x i64] zeroinitializer
-// T100GEN: @[[T100C:__profc__Z4loopILj100EEvv]] = linkonce_odr {{(hidden|dso_local)}} global [2 x i64] zeroinitializer
+// The linkage can be target dependent, so accept 

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

[Jinsong Ji via Phabricator via cfe-commits]

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 merge the csects for profiling data/counter
sections.

AIX binder now get the namedcsect support to support the merging,
so now we can enable PGO without LTO with the new binder.


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D110422

Files:
  clang/lib/Driver/ToolChains/AIX.cpp
  clang/lib/Driver/ToolChains/Clang.cpp
  clang/test/Driver/unsupported-option.c
  clang/test/Profile/cxx-templates.cpp
  llvm/lib/Transforms/Instrumentation/InstrProfiling.cpp
  llvm/test/Instrumentation/InstrProfiling/profiling.ll

Index: llvm/test/Instrumentation/InstrProfiling/profiling.ll
===
--- llvm/test/Instrumentation/InstrProfiling/profiling.ll
+++ llvm/test/Instrumentation/InstrProfiling/profiling.ll
@@ -45,8 +45,8 @@
 ; MACHO: @__profd_foo_weak = weak hidden global
 ; COFF: @__profc_foo_weak = weak hidden global
 ; COFF: @__profd_foo_weak = private global
-; XCOFF: @__profc_foo_weak = weak hidden global
-; XCOFF: @__profd_foo_weak = weak hidden global
+; XCOFF: @__profc_foo_weak = private global
+; XCOFF: @__profd_foo_weak = private global
 define weak void @foo_weak() {
   call void @llvm.instrprof.increment(i8* getelementptr inbounds ([8 x i8], [8 x i8]* @__profn_foo_weak, i32 0, i32 0), i64 0, i32 1, i32 0)
   ret void
@@ -71,8 +71,8 @@
 ; MACHO: @__profd_foo_inline = linkonce_odr hidden global
 ; COFF: @__profc_foo_inline = linkonce_odr hidden global{{.*}} section ".lprfc$M", align 8
 ; COFF: @__profd_foo_inline = private global{{.*}} section ".lprfd$M", align 8
-; XCOFF: @__profc_foo_inline = linkonce_odr hidden global
-; XCOFF: @__profd_foo_inline = linkonce_odr hidden global
+; XCOFF: @__profc_foo_inline = private global
+; XCOFF: @__profd_foo_inline = private global
 define linkonce_odr void @foo_inline() {
   call void @llvm.instrprof.increment(i8* getelementptr inbounds ([10 x i8], [10 x i8]* @__profn_foo_inline, i32 0, i32 0), i64 0, i32 1, i32 0)
   ret void
@@ -84,8 +84,8 @@
 ; MACHO: @__profd_foo_extern = linkonce_odr hidden global
 ; COFF: @__profc_foo_extern = linkonce_odr hidden global {{.*}}section ".lprfc$M", comdat, align 8
 ; COFF: @__profd_foo_extern = private global {{.*}}section ".lprfd$M", comdat($__profc_foo_extern), align 8
-; XCOFF: @__profc_foo_extern = linkonce_odr hidden global
-; XCOFF: @__profd_foo_extern = linkonce_odr hidden global
+; XCOFF: @__profc_foo_extern = private global
+; XCOFF: @__profd_foo_extern = private global
 define available_externally void @foo_extern() {
   call void @llvm.instrprof.increment(i8* getelementptr inbounds ([10 x i8], [10 x i8]* @__profn_foo_extern, i32 0, i32 0), i64 0, i32 1, i32 0)
   ret void
Index: llvm/lib/Transforms/Instrumentation/InstrProfiling.cpp
===
--- llvm/lib/Transforms/Instrumentation/InstrProfiling.cpp
+++ llvm/lib/Transforms/Instrumentation/InstrProfiling.cpp
@@ -862,6 +862,15 @@
   GlobalValue::LinkageTypes Linkage = NamePtr->getLinkage();
   GlobalValue::VisibilityTypes Visibility = NamePtr->getVisibility();
 
+  // 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,
+  // 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.
+  if (TT.isOSBinFormatXCOFF()) {
+Linkage = GlobalValue::PrivateLinkage;
+Visibility = GlobalValue::DefaultVisibility;
+  }
   // Move the name variable to the right section. Place them in a COMDAT group
   // if the associated function is a COMDAT. This will make sure that only one
   // copy of counters of the COMDAT function will be emitted after linking. Keep
@@ -966,7 +975,7 @@
   // that other copies must have the same CFG and cannot have value profiling.
   // If no hash suffix, other profd copies may be referenced by code.
   if (NS == 0 && !(DataReferencedByCode && NeedComdat && !Renamed) &&
-  (TT.isOSBinFormatELF() ||
+  (TT.isOSBinFormatELF() || TT.isOSBinFormatXCOFF() ||
(!DataReferencedByCode && TT.isOSBinFormatCOFF( {
 Linkage = GlobalValue::PrivateLinkage;
 Visibility = GlobalValue::DefaultVisibility;
Index: clang/test/Profile/cxx-templates.cpp
===
--- clang/test/Profile/cxx-templates.cpp
+++ clang/test/Profile/cxx-templates.cpp
@@ -10,8 +10,10 @@
 // RUN: FileCheck --input-file=%tuse -check-prefix=T0USE -check-prefix=ALL %s
 // RUN: FileCheck --input-file=%tuse -check-prefix=T100USE