Re: [PATCH, rs6000] PR 66337: Improve Compliance with Power ABI

2016-02-18 Thread David Edelsohn
> Ulrich Weigand wrote:
>> Kevin Nilsen wrote:
>
>> This patch has bootstrapped and tested on powerpc64le-unknown-linux-gnu and=
>>  powerpc64be-unknown-linux-gnu (both 32-bit and 64-bit) and=20
>> powerpc64-unknown-freebsd11.0 (big endian) with no regressions.  Is it ok to
>> fix this on the trunk?
>>
>> The problem described in PR66337 (https://gcc.gnu.org/bugzilla/show_bug.cgi=
>> ?id=3D3D66337) is that compiling for PowerPC targets with the -malign-power=
>>  command-line option results in invalid field offsets for certain structure=
>>  members.   As identified in the problem report, this results from a macro =
>> definition present in both config/rs6000/{freebsd64,linux64}.h, which in bo=
>> th cases is introduced by the comment:
>>
>> /* PowerPC64 Linux word-aligns FP doubles when -malign-power is given. */
>>
>> I have consulted various ABI documents, including "64-bit PowerPC ELF Appli=
>> cation Binary Interface Supplement 1.9" (http://refspecs.linuxfoundation.or=
>> g/ELF/ppc64/PPC-elf64abi.html), "Power Architecture 64-Bit ELF V2 ABI Speci=
>> fication" (https://members.openpowerfoundation.org/document/dl/576), and "P=
>> ower
>> Architecture(R) 32-bit Application Binary Interface Supplement 1.0 - Linux(=
>> R) & Embedded" (https://www.power.org/documentation/power-architecture-32-b=
>> it-abi-supplement-1-0-embeddedlinuxunified/).  I have not been able to find=
>>  any basis for this comment and thus am concluding that the comment and exi=
>> sting implementation are incorrect.
>>
>> The implemented patch removes the comment and changes the macro definition =
>> so that field alignment calculations on 64-bit architectures ignore the -ma=
>> lign-power command-line option.  With this fix, the test case identified in=
>>  the PR behaves as was expected by the submitter.
>
> There seems to be some confusion here.  First of all, on Linux and FreeBSD,
> the *default* behavior is -malign-natural, which matches what the Linux ABI
> specifies.  Using -malign-power on *Linux* is an explicit instruction to
> the compiler to *deviate* from the documented ABI.
>
> The only effect that the deviation has on Linux is to change the alignment
> requirements for certain structure elements.  Your patch removes this change,
> making -malign-power fully a no-op on Linux and FreeBSD.  This doesn't seem
> to be particularly useful ...  If you don't want the effect, you can simply
> not use that switch.
>
> To my understanding, the intent of providing that switch was to allow
> creating code that is compatible code produced by some other compilers
> that do not adhere to the Linux ABI, but some other ABI.  In particular,
> my understanding is that the *AIX* ABI has these alignment requirements.
> And in fact, GCC on *AIX* defaults to -malign-power.
>
> Looking at PR 66337, the submitter actually refers to the behaviour of
> GCC on AIX, so I'm not sure how Linux is even relevant here.  (Maybe
> there is something wrong in how GCC implements the AIX ABI.  But I'm
> not really familar with AIX, so I can't help much with that.)

AIX does not use natural alignment.  For historical reasons, the
maximum alignment of double is word alignment.  In an attempt to
correct the alignment mistake, the AIX POWER ABI increases the
alignment of structures who first element is double to double word.
XLC increases the alignment of the member but GCC does not.

GCC allows use of AIX POWER ABI alignment in ELF for some early
customers.  The option has nothing to do with Linux ABIs nor embedded
ABIs.

Thanks for the patch, but it is not addressing the correct problem.
The issue is specifically about GCC compatibility with XLC for AIX
ABI.

Thanks, David


Re: [PATCH, rs6000] PR 66337: Improve Compliance with Power ABI

2016-02-18 Thread Ulrich Weigand
Kevin Nilsen wrote:

> This patch has bootstrapped and tested on powerpc64le-unknown-linux-gnu and=
>  powerpc64be-unknown-linux-gnu (both 32-bit and 64-bit) and=20
> powerpc64-unknown-freebsd11.0 (big endian) with no regressions.  Is it ok t=
> o fix this on the trunk?
> 
> The problem described in PR66337 (https://gcc.gnu.org/bugzilla/show_bug.cgi=
> ?id=3D3D66337) is that compiling for PowerPC targets with the -malign-power=
>  command-line option results in invalid field offsets for certain structure=
>  members.   As identified in the problem report, this results from a macro =
> definition present in both config/rs6000/{freebsd64,linux64}.h, which in bo=
> th cases is introduced by the comment:
> 
> /* PowerPC64 Linux word-aligns FP doubles when -malign-power is given. */
> 
> I have consulted various ABI documents, including "64-bit PowerPC ELF Appli=
> cation Binary Interface Supplement 1.9" (http://refspecs.linuxfoundation.or=
> g/ELF/ppc64/PPC-elf64abi.html), "Power Architecture 64-Bit ELF V2 ABI Speci=
> fication" (https://members.openpowerfoundation.org/document/dl/576), and "P=
> ower
> Architecture(R) 32-bit Application Binary Interface Supplement 1.0 - Linux(=
> R) & Embedded" (https://www.power.org/documentation/power-architecture-32-b=
> it-abi-supplement-1-0-embeddedlinuxunified/).  I have not been able to find=
>  any basis for this comment and thus am concluding that the comment and exi=
> sting implementation are incorrect.
> 
> The implemented patch removes the comment and changes the macro definition =
> so that field alignment calculations on 64-bit architectures ignore the -ma=
> lign-power command-line option.  With this fix, the test case identified in=
>  the PR behaves as was expected by the submitter.

There seems to be some confusion here.  First of all, on Linux and FreeBSD,
the *default* behavior is -malign-natural, which matches what the Linux ABI
specifies.  Using -malign-power on *Linux* is an explicit instruction to
the compiler to *deviate* from the documented ABI.

The only effect that the deviation has on Linux is to change the alignment
requirements for certain structure elements.  Your patch removes this change,
making -malign-power fully a no-op on Linux and FreeBSD.  This doesn't seem
to be particularly useful ...  If you don't want the effect, you can simply
not use that switch.


To my understanding, the intent of providing that switch was to allow
creating code that is compatible code produced by some other compilers
that do not adhere to the Linux ABI, but some other ABI.  In particular,
my understanding is that the *AIX* ABI has these alignment requirements.
And in fact, GCC on *AIX* defaults to -malign-power.

Looking at PR 66337, the submitter actually refers to the behaviour of
GCC on AIX, so I'm not sure how Linux is even relevant here.  (Maybe
there is something wrong in how GCC implements the AIX ABI.  But I'm
not really familar with AIX, so I can't help much with that.)


Bye,
Ulrich

-- 
  Dr. Ulrich Weigand
  GNU/Linux compilers and toolchain
  ulrich.weig...@de.ibm.com



[PATCH, rs6000] PR 66337: Improve Compliance with Power ABI

2016-02-18 Thread Kelvin Nilsen
This patch has bootstrapped and tested on powerpc64le-unknown-linux-gnu and 
powerpc64be-unknown-linux-gnu (both 32-bit and 64-bit) and 
powerpc64-unknown-freebsd11.0 (big endian) with no regressions.  Is it ok to 
fix this on the trunk?

The problem described in PR66337 
(https://gcc.gnu.org/bugzilla/show_bug.cgi?id=3D66337) is that compiling for 
PowerPC targets with the -malign-power command-line option results in invalid 
field offsets for certain structure members.   As identified in the problem 
report, this results from a macro definition present in both 
config/rs6000/{freebsd64,linux64}.h, which in both cases is introduced by the 
comment:

/* PowerPC64 Linux word-aligns FP doubles when -malign-power is given. */

I have consulted various ABI documents, including "64-bit PowerPC ELF 
Application Binary Interface Supplement 1.9" 
(http://refspecs.linuxfoundation.org/ELF/ppc64/PPC-elf64abi.html), "Power 
Architecture 64-Bit ELF V2 ABI Specification" 
(https://members.openpowerfoundation.org/document/dl/576), and "Power
Architecture(R) 32-bit Application Binary Interface Supplement 1.0 - Linux(R) & 
Embedded" 
(https://www.power.org/documentation/power-architecture-32-bit-abi-supplement-1-0-embeddedlinuxunified/).
  I have not been able to find any basis for this comment and thus am 
concluding that the comment and existing implementation are incorrect.

The implemented patch removes the comment and changes the macro definition so 
that field alignment calculations on 64-bit architectures ignore the 
-malign-power command-line option.  With this fix, the test case identified in 
the PR behaves as was expected by the submitter.

gcc/ChangeLog:

2016-02-17  Kelvin Nilsen  

PR target/66337
* config/rs6000/freebsd64.h: Remove erroneous comment and correct
definition of ADJUST_FIELD_ALIGN macro.
* config/rs6000/linux64.h: Remove erroneous comment and correct
definition of ADJUST_FIELD_ALIGN macro.

gcc/testsuite/ChangeLog:

2016-02-17  Kelvin Nilsen  

PR target/66337
* g++.dg/pr66337-1.C: New test.
Index: gcc/testsuite/g++.dg/pr66337-1.C
===
--- gcc/testsuite/g++.dg/pr66337-1.C(revision 0)
+++ gcc/testsuite/g++.dg/pr66337-1.C(revision 233507)
@@ -0,0 +1,77 @@
+/* { dg-do run { target { powerpc*-*-* } } } */
+/* { dg-options "-std=c++11 -malign-power -O2" } */
+
+/* Power ABI for 32-bit and 64-bit compilers place the same alignment
+   restrictions on long longs and doubles. */
+
+typedef double _Tp;
+
+struct _Tp2 { 
+  char b;
+  int i;
+  char c;
+  long long l;
+  _Tp _M_t; 
+};
+
+extern _Tp2 tp2e;
+
+struct _ST1 {
+  char a;
+};
+
+struct _ST2 {
+  int b;
+};
+
+struct _ST3 {
+  float f;
+};
+
+struct _ST4 {
+  double d;
+};
+
+struct _ST5 {
+  char a;
+  double d;
+};
+
+struct _ST6 {
+  double d;
+  char a;
+};
+
+int main ()
+{
+  int a = alignof (_Tp2);
+  int b = __alignof__(_Tp2::_M_t);
+  int c = alignof(_Tp);
+  int d = __alignof__(tp2e._M_t);
+  int e = alignof(_Tp2::_M_t);
+
+  int f = __alignof__(_Tp2::l);
+  int g = alignof (long long);
+  int h = __alignof__(tp2e.l);
+  int i = alignof(_Tp2::l);
+
+  if ((a != 8) || (b != 8) || (c != 8) || (d != 8) || (e != 8) 
+  || (f != 8) || (g != 8) || (h != 8) || (i != 8))
+return -1;
+
+  a = sizeof (_ST1);
+  b = sizeof (_ST2);
+  c = sizeof (_ST3);
+  d = sizeof (_ST4);
+  e = sizeof (_ST5);
+  f = sizeof (_ST6);
+  g = sizeof (_Tp2);
+
+  if ((a != 1) || (b != 4) || (c != 4) || (d != 8) 
+  || (e != 16) || (f != 16) || (g != 32))
+return -2;
+
+  /* success */
+  return 0;
+}
+  
Index: gcc/config/rs6000/freebsd64.h
===
--- gcc/config/rs6000/freebsd64.h   (revision 233308)
+++ gcc/config/rs6000/freebsd64.h   (working copy)
@@ -363,16 +363,10 @@ extern int dot_symbols;
 /* Use standard DWARF numbering for DWARF debugging information.  */
 #define RS6000_USE_DWARF_NUMBERING
 
-/* PowerPC64 Linux word-aligns FP doubles when -malign-power is given.  */
 #undef  ADJUST_FIELD_ALIGN
 #define ADJUST_FIELD_ALIGN(FIELD, COMPUTED) \
   (rs6000_special_adjust_field_align_p ((FIELD), (COMPUTED))   \
-   ? 128\
-   : (TARGET_64BIT  \
-  && TARGET_ALIGN_NATURAL == 0  \
-  && TYPE_MODE (strip_array_types (TREE_TYPE (FIELD))) == DFmode)   \
-   ? MIN ((COMPUTED), 32)   \
-   : (COMPUTED))
+   ? 128: (COMPUTED))
 
 #undef  TOC_SECTION_ASM_OP
 #define TOC_SECTION_ASM_OP \
Index: gcc/config/rs6000/linux64.h
===
--- gcc/config/rs6000/linux64.h (revision 233308)
+++ gcc/config/rs6000/linux64.h (working copy)
@@ -290,16 +290,10 @@ extern int