This is the mail archive of the
binutils@sourceware.org
mailing list for the binutils project.
Re: [PATCH] Two level comdat priorities in gold
- From: Sriraman Tallam <tmsriram at google dot com>
- To: Rafael EspÃndola <rafael dot espindola at gmail dot com>
- Cc: binutils <binutils at sourceware dot org>, Cary Coutant <ccoutant at gmail dot com>, David Li <davidxl at google dot com>, Paul Pluzhnikov <ppluzhnikov at google dot com>, Ian Lance Taylor <iant at google dot com>, Daniel Berlin <dannyb at google dot com>, Eric Christopher <echristo at gmail dot com>
- Date: Tue, 21 Jul 2015 10:22:17 -0700
- Subject: Re: [PATCH] Two level comdat priorities in gold
- Authentication-results: sourceware.org; auth=none
- References: <CAAs8HmwHCWKf+Onx=ERLgLpk6276f+jhuW-WiKpvhz6QDxWQ2Q at mail dot gmail dot com> <CAG3jRe+yMffr+4BgV3=ErZTC7U3BmfenVEe=AfDzvHyxO+EVYQ at mail dot gmail dot com>
On Tue, Jul 21, 2015 at 6:00 AM, Rafael EspÃndola
<rafael.espindola@gmail.com> wrote:
> Lets say a few files use std::vector<int>. Two are compiled for avx,
> the rest for sse.
>
> It would still be nice to for the avx files to use avx enabled
> versions of the std::vector<int> functions, even if they are not
> inlined. If I understand the current scheme sse would be used.
>
> Making the avx files use internal versions of the std::vector<int>
> would solve the problem, but bloat the output if more than one file
> uses avx.
>
> What about optionally including the cpu requirement in the mangling
> and having, for example,_ZNKSt6vectorIiSaIiEE4sizeEv.avx in addition
> to _ZNKSt6vectorIiSaIiEE4sizeEv? They would not be pointer equal, but
> the code would be using a non default option anyway. This is similar
> to -fvisibility-inlines-hidden.
Isnt this equivalent to localizing the comdat functions (especially
those containing specialized instructions) to that module compiled
with -mxxx. I tried this here:
https://gcc.gnu.org/ml/gcc-patches/2015-06/msg01174.html
Breaking the pointer inequality and this not working for virtual
comdat functions are the two big problems with this patch.
Thanks
Sri
>
> Cheers,
> Rafael
>
> On 13 July 2015 at 19:17, Sriraman Tallam <tmsriram@google.com> wrote:
>> Hi,
>>
>> This patch implements a two level comdat function priority. Comdats defined
>> in those objects with a named section ".gnu.comdat.low" are treated with a
>> lower priority. If this patch is suitable, I could add compiler option
>> -fcomdat-priority-low which creates this named section in the object file. For
>> testing, I have used the asm directive to create this named section. Comdat
>> priority checking is enabled and honored with new option
>> --honor-comdat-priority.
>>
>> Context: I have created this patch to support Module Level Multiversioning.
>> Multi versioning at module granularity is done by compiling a subset of modules
>> with advanced ISA instructions, supported on later generations of the target
>> architecture, via -m<isa> options and invoking the functions defined in these
>> modules with explicit checks for the ISA support via builtin functions,
>> __builtin_cpu_supports. This mechanism has the unfortunate side-effect that
>> generated COMDAT candidates from these modules can contain these advanced
>> instructions and potentially âviolateâ ODR assumptions. Choosing such a COMDAT
>> candidate over a generic one from a different module can cause SIGILL on
>> platforms where the advanced ISA is not supported.
>>
>> Here is a slightly contrived example to illustrate:
>>
>>
>> matrixdouble.h
>> --------------------
>> // Template (Comdat) function definition in a header:
>>
>> template<typename T>
>> __attribute__((noinline))
>> void matrixDouble (T *a) {
>> for (int i = 0 ; i < 16; ++i) //Vectorizable Loop
>> a[i] = a[i] * 2;
>> }
>>
>> avx.cc (Compile with -mavx -O2)
>> ---------
>>
>> #include "matrixdouble.h"
>> void getDoubleAVX(int *a) {
>> matrixDouble(a); // Instantiated with vectorized AVX instructions
>> }
>>
>>
>> non_avx.cc (Compile with -mno-avx -O2)
>> ---------------
>>
>> #include âmatrixdouble.hâ
>> void
>> getDouble(int *a) {
>> matrixDouble(a); // Instantiated with non-AVX instructions
>> }
>>
>>
>> main.cc
>> -----------
>>
>> void getDoubleAVX(int *a);
>> void getDouble(int *a);
>>
>> int a[] = { 0, 1, 2, 3, 4, 5, 6, 7, 8, 9, 10, 11, 12, 13, 14, 15 };
>> int main () {
>> // The AVX call is appropriately guarded.
>> if (__builtin_cpu_supports(âavxâ))
>> getDoubleAVX(a);
>> else
>> getDouble(a);
>> return a[0];
>> }
>>
>> In the above code, function âgetDoubleAVXâ is only called when the
>> run-time CPU supports AVX instructions. This code looks clean but
>> suffers from the COMDAT ODR violation. Two copies of COMDAT function
>> âmatrixDoubleâ are generated. One copy is generated in object file
>> âavx.oâ with AVX instructions and another copy exists in ânon_avx.oâ
>> without AVX instruction. At link time, in a link order where object
>> file avx.o is seen ahead of non_avx.o, the COMDAT copy of function
>> âmatrixDoubleâ that contains AVX instructions is kept leading to
>> SIGILL on unsupported platforms. To reproduce the SIGILL,
>>
>>
>> $ g++ -c -O2 -mavx avx.cc
>> $ g++ -c -O2 -mno-avx non_avx.cc
>> $ g++ main.cc avx.o non_avx.o
>> $ ./a.out # on a non-AVX machine
>> Illegal Instruction
>>
>>
>> Comdat priorities can solve the above problem by tagging the avx.cc file as
>> having lower priority comdats.
>>
>> Patch implementation details:
>>
>> Thanks to the two-pass approach available in do_layout (object.cc), implementing
>> two level comdat priority is easy. The first pass processes all default
>> priority comdat groups and the second pass then processes the lower priority
>> comdat groups. This ensures that a lower priority candidate is never picked
>> when a default priority substitute is available.
>>
>> * gold.cc (queue_middle_tasks): Check if comdat priority is enabled.
>> * object.cc (do_layout): Do it as two pass if comdat priority is
>> enabled. Check for low priority comdats. Do not process low priority
>> comdat groups in the first pass.
>> * object.h (Object::Object): Initialize has_low_priority_comdats_
>> (Object::has_low_priority_comdats): New function.
>> (Object::set_has_low_priority_comdats): New function
>> (Object::has_low_priority_comdats_): New member.
>> * options.h (--honor-comdat-priority): New option.
>> * resolve.cc (Symbol_table::higher_priority_comdat_symbol): New function.
>> (should_override): Check for a higher priority comdat symbol.
>> * symtab.h (Symbol_table::higher_priority_comdat_symbol): New function.
>> * testsuite/Makefile.am (comdat_priority_test): New test.
>> * testsuite/comdat_priority_test_default.cc: New file.
>> * testsuite/comdat_priority_test_low.cc: New file.
>>
>>
>> Is this reasonable?
>>
>> Patch attached.
>>
>> Thanks
>> Sri