This is the mail archive of the binutils@sourceware.org mailing list for the binutils project.


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]
Other format: [Raw text]

Re: [PATCH] Two level comdat priorities in gold


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


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]