Re: [RFC] COMDAT Safe Module Level Multi versioning

[Sriraman Tallam <tmsriram@google.com>]

On Tue, May 19, 2015 at 2:39 AM, Richard Biener
<richard.guenther@gmail.com> wrote:
> On Tue, May 19, 2015 at 8:16 AM, Sriraman Tallam <tmsriram@google.com> wrote:
>> We have the following problem with selectively compiling modules with
>> -m<isa> options and I have provided a solution to solve this.  I would
>> like to hear what you think.
>>
>> 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
>>
>>
>> To solve this, I propose introducing a new compiler option, say
>> -fodr-unsafe-comdats, to let the user tag objects that use specialized
>> options and let the linker choose the comdat candidate to be linked
>> wisely.  The root cause of the above problem is that comdat functions
>> in common headers may not be properly guarded and the linker picks the
>> first candidate it sees.  A link order where the object with the
>> specialized comdat functions appear first causes these comdats to be
>> picked leading to SIGILL on unsupported arches.  With the objects
>> tagged, the linker can be made to pick other comdat candidates when
>> possible.
>>
>> More details:
>>
>> This option is user specified when using arch specific options like
>> -m<isa>.  It is an indicator to the compiler that any comdat bodies
>> generated are potentially unsafe for execution.  Note that the COMDAT
>> bodies however have to be generated as there are no guarantees that
>> other modules will do so.  The compiler then emits a specially named
>> section, like “.gnu.odr.unsafe”, in the object file.  When the linker
>> tries to pick a COMDAT candidate from several choices, it must avoid
>> COMDAT copies from objects with sections named “.gnu.odr.unsafe” when
>> presented with a choice to pick a candidate from an object that does
>> not have the “.gnu.odr.unsafe” section.  Note that it may not be
>> possible to do that in which case the linker must pick the unsafe
>> copy, it could explicitly warn when this happens.
>>
>> Alternately,  the compiler can bind locally any emitted comdat version
>> from a specialized module, which could also be guarded by an option.
>> This will solve the problem but this may not be always possible
>> especially when addresses of any such comdat version is taken.
>
> Hm.  But which options are unsafe?  Also wouldn't it be better to simp

In general, should that be any option that affects code gen and is
only *applied to a subset of modules* is potentially unsafe as the
comdat copies generated from those modules are not identical to the
copies from other modules.  Tagging such modules with
-fodr-unsafe-comdats, even conservatively, is fine.  In the worst
case, the comdat candidate from that module is the only available
candidate and the linker uses that and emits a non-fatal message that
this comdat was used.  Shouldn't that be alright?

Thanks
Sri


> _not_ have unsafe options produce comdats but always make local clones
> for them (thus emit the comdat with "unsafe" flags dropped)?
>
> Richard.
>
>>
>> Thanks
>> Sri