[RFC] Adding AIX power alignment rule in clang front end

classic Classic list List threaded Threaded
7 messages Options
Reply | Threaded
Open this post in threaded view
|

[RFC] Adding AIX power alignment rule in clang front end

Fangrui Song via cfe-dev

Currently we are trying to implement AIX special alignment rule on the top of Itanium ABI in Clang. On AIX,  in aggregates, the first member of double/long double is aligned according to its natural alignment value; subsequent double/long double of the aggregate are aligned on 4-byte boundaries.

 

This rule is recursive. It applies all the way to the inner most nested members. An example is as the following:

 

struct D {
        double d;
        int i;
};

 

struct IntFirst {
        int i;
        struct D d;
};

 

The size of struct D is 16, class alignment is 8;

The size of struct IntFirst is 20, class alignment is 4;

 

So our current idea is to have an alignment field to record nested struct info and pass the special alignment value all the way back to the ourter most aggregate.

 

And based on this idea, there are two possible ways.

 

1.

The first one is that we introduced a new field in class ASTRecordLayout

Called `AIXOffsetAlignment`  which represents the special AIX alignment for the object that

contains floating-point member or sub-member. This is for AIX-ABI only.

 

The proposed change has been put on Phabricator under review: https://reviews.llvm.org/D78563

 

Pros:

- The design is cleaner. It is clear that such field is for AIX and it does not affect any other target

   in terms of functionality.



Cons:

- All other targets using ItaniumRecordLayoutBuilder will also have to pay the price to keep

   updating this `AIXOffsetAlignment` along the common code path by using `UpdateAlignment`.

 

2.

The second possible way we have in mind is to overload the usage of current Microsoft-ABI only alignment field `RequiredAlignment` and rename it to `TargetSpecialAlignment` instead of the above first way to create a new alignment field.

This alignment field will function the same in Itanium ABI part for AIX. Meanwhile, because the current `RequiredAlignment` is only used in MS code, renaming will not affect MS code path functionally.

 

Pro:

- Class ASTRecordLayout does not need to construct one more field `AIXOffsetAlignment` compared to the method one.

- Instead of having each target add new fields for their special alignment rules, we could have one field that handles all the potential alignment differences for every target.

 

Cons:

-  Still, all other targets using ItaniumRecordLayoutBuilder will also have to pay the price to   

   keep updating this `TargetSpecialAlignment` along the common code path by using

   `UpdateAlignment`.

-  By overloading the usage, it may create confusion when more targets try to utilize it in the

   future.

-  The degree of willingness of sharing this current MS-only alignment field is unknown.

 

 

I would like to see how people think about the above two ways. Which one is better? Any concerns about either one? Or any other ideas to suggest?

 

Please let me know if there are any questions about my current proposal and design. Your feedback is appreciated.

 

Regards,

 

Xiangling Liao


_______________________________________________
cfe-dev mailing list
[hidden email]
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-dev
Reply | Threaded
Open this post in threaded view
|

Re: [RFC] Adding AIX power alignment rule in clang front end

Fangrui Song via cfe-dev

Assuming I’m understanding the rule correctly, it doesn’t actually affect “alignment” of the type in the sense we would normally understand it.  A struct with a double as the first member can be stored at an address with four-byte alignment.  The rule only involves inserting extra padding at the end of the struct. In particular, from the way you’re describing the rule, I think the special alignment rule isn’t supposed to affect the result of _Alignof.  Does that seem right?

 

Given that, do you actually need to store anything in the RecordLayout at all?  You could just write a check like “if the first member of the struct is a double (or recursively, a struct where the first member is a double), round up the size of the struct to a multiple of 8”.

 

Also, probably worth checking the layout for a testcase like the following, to check the interaction between the extra padding and the nvsize:

 

class A { double a; int b; };

class B : A { int c; };

 

-Eli

 

From: cfe-dev <[hidden email]> On Behalf Of Xiangling Liao via cfe-dev
Sent: Friday, April 24, 2020 6:21 AM
To: [hidden email]
Subject: [EXT] [cfe-dev] [RFC] Adding AIX power alignment rule in clang front end

 

Currently we are trying to implement AIX special alignment rule on the top of Itanium ABI in Clang. On AIX,  in aggregates, the first member of double/long double is aligned according to its natural alignment value; subsequent double/long double of the aggregate are aligned on 4-byte boundaries.

 

This rule is recursive. It applies all the way to the inner most nested members. An example is as the following:

 

struct D {
        double d;
        int i;
};

 

struct IntFirst {
        int i;
        struct D d;
};

 

The size of struct D is 16, class alignment is 8;

The size of struct IntFirst is 20, class alignment is 4;

 

So our current idea is to have an alignment field to record nested struct info and pass the special alignment value all the way back to the ourter most aggregate.

 

And based on this idea, there are two possible ways.

 

1.

The first one is that we introduced a new field in class ASTRecordLayout

Called `AIXOffsetAlignment`  which represents the special AIX alignment for the object that

contains floating-point member or sub-member. This is for AIX-ABI only.

 

The proposed change has been put on Phabricator under review: https://reviews.llvm.org/D78563

 

Pros:

- The design is cleaner. It is clear that such field is for AIX and it does not affect any other target

   in terms of functionality.

 

Cons:

- All other targets using ItaniumRecordLayoutBuilder will also have to pay the price to keep

   updating this `AIXOffsetAlignment` along the common code path by using `UpdateAlignment`.

 

2.

The second possible way we have in mind is to overload the usage of current Microsoft-ABI only alignment field `RequiredAlignment` and rename it to `TargetSpecialAlignment` instead of the above first way to create a new alignment field.

This alignment field will function the same in Itanium ABI part for AIX. Meanwhile, because the current `RequiredAlignment` is only used in MS code, renaming will not affect MS code path functionally.

 

Pro:

- Class ASTRecordLayout does not need to construct one more field `AIXOffsetAlignment` compared to the method one.

- Instead of having each target add new fields for their special alignment rules, we could have one field that handles all the potential alignment differences for every target.

 

Cons:

-  Still, all other targets using ItaniumRecordLayoutBuilder will also have to pay the price to   

   keep updating this `TargetSpecialAlignment` along the common code path by using

   `UpdateAlignment`.

-  By overloading the usage, it may create confusion when more targets try to utilize it in the

   future.

-  The degree of willingness of sharing this current MS-only alignment field is unknown.

 

 

I would like to see how people think about the above two ways. Which one is better? Any concerns about either one? Or any other ideas to suggest?

 

Please let me know if there are any questions about my current proposal and design. Your feedback is appreciated.

 

Regards,

 

Xiangling Liao


_______________________________________________
cfe-dev mailing list
[hidden email]
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-dev
Reply | Threaded
Open this post in threaded view
|

Re: [RFC] Adding AIX power alignment rule in clang front end

Fangrui Song via cfe-dev
I think what Eli said makes sense.

I'd add that maybe the concept of "preferred alignment" is closer to what you want.

On Fri, Apr 24, 2020 at 12:55 PM Eli Friedman via cfe-dev <[hidden email]> wrote:

Assuming I’m understanding the rule correctly, it doesn’t actually affect “alignment” of the type in the sense we would normally understand it.  A struct with a double as the first member can be stored at an address with four-byte alignment.  The rule only involves inserting extra padding at the end of the struct. In particular, from the way you’re describing the rule, I think the special alignment rule isn’t supposed to affect the result of _Alignof.  Does that seem right?

 

Given that, do you actually need to store anything in the RecordLayout at all?  You could just write a check like “if the first member of the struct is a double (or recursively, a struct where the first member is a double), round up the size of the struct to a multiple of 8”.

 

Also, probably worth checking the layout for a testcase like the following, to check the interaction between the extra padding and the nvsize:

 

class A { double a; int b; };

class B : A { int c; };

 

-Eli

 

From: cfe-dev <[hidden email]> On Behalf Of Xiangling Liao via cfe-dev
Sent: Friday, April 24, 2020 6:21 AM
To: [hidden email]
Subject: [EXT] [cfe-dev] [RFC] Adding AIX power alignment rule in clang front end

 

Currently we are trying to implement AIX special alignment rule on the top of Itanium ABI in Clang. On AIX,  in aggregates, the first member of double/long double is aligned according to its natural alignment value; subsequent double/long double of the aggregate are aligned on 4-byte boundaries.

 

This rule is recursive. It applies all the way to the inner most nested members. An example is as the following:

 

struct D {
        double d;
        int i;
};

 

struct IntFirst {
        int i;
        struct D d;
};

 

The size of struct D is 16, class alignment is 8;

The size of struct IntFirst is 20, class alignment is 4;

 

So our current idea is to have an alignment field to record nested struct info and pass the special alignment value all the way back to the ourter most aggregate.

 

And based on this idea, there are two possible ways.

 

1.

The first one is that we introduced a new field in class ASTRecordLayout

Called `AIXOffsetAlignment`  which represents the special AIX alignment for the object that

contains floating-point member or sub-member. This is for AIX-ABI only.

 

The proposed change has been put on Phabricator under review: https://reviews.llvm.org/D78563

 

Pros:

- The design is cleaner. It is clear that such field is for AIX and it does not affect any other target

   in terms of functionality.

 

Cons:

- All other targets using ItaniumRecordLayoutBuilder will also have to pay the price to keep

   updating this `AIXOffsetAlignment` along the common code path by using `UpdateAlignment`.

 

2.

The second possible way we have in mind is to overload the usage of current Microsoft-ABI only alignment field `RequiredAlignment` and rename it to `TargetSpecialAlignment` instead of the above first way to create a new alignment field.

This alignment field will function the same in Itanium ABI part for AIX. Meanwhile, because the current `RequiredAlignment` is only used in MS code, renaming will not affect MS code path functionally.

 

Pro:

- Class ASTRecordLayout does not need to construct one more field `AIXOffsetAlignment` compared to the method one.

- Instead of having each target add new fields for their special alignment rules, we could have one field that handles all the potential alignment differences for every target.

 

Cons:

-  Still, all other targets using ItaniumRecordLayoutBuilder will also have to pay the price to   

   keep updating this `TargetSpecialAlignment` along the common code path by using

   `UpdateAlignment`.

-  By overloading the usage, it may create confusion when more targets try to utilize it in the

   future.

-  The degree of willingness of sharing this current MS-only alignment field is unknown.

 

 

I would like to see how people think about the above two ways. Which one is better? Any concerns about either one? Or any other ideas to suggest?

 

Please let me know if there are any questions about my current proposal and design. Your feedback is appreciated.

 

Regards,

 

Xiangling Liao

_______________________________________________
cfe-dev mailing list
[hidden email]
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-dev

_______________________________________________
cfe-dev mailing list
[hidden email]
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-dev
Reply | Threaded
Open this post in threaded view
|

Re: [RFC] Adding AIX power alignment rule in clang front end

Fangrui Song via cfe-dev
In reply to this post by Fangrui Song via cfe-dev
On Fri, Apr 24, 2020 at 3:55 PM Eli Friedman via cfe-dev <[hidden email]> wrote:

Assuming I’m understanding the rule correctly, it doesn’t actually affect “alignment” of the type in the sense we would normally understand it.  A struct with a double as the first member can be stored at an address with four-byte alignment.  The rule only involves inserting extra padding at the end of the struct. In particular, from the way you’re describing the rule, I think the special alignment rule isn’t supposed to affect the result of _Alignof.  Does that seem right?

The reference implementations on AIX indicate that the result of _Alignof is affected.

C:
typedef struct A { double x; int y; } A;
extern char x[_Alignof(A)];
extern char x[8];

C++:
struct A {
  A(const A &);
  double x; int y;
};
struct B : A { int z; };

extern char x[sizeof(B)];
extern char x[16];

extern char y[alignof(A)];
extern char y[8];
 

 

Given that, do you actually need to store anything in the RecordLayout at all?

Does the new information change your analysis or it is still worthwhile to internally treat the alignment as the lower value (and add special code to introduce the padding-for-preferred-alignment)?
 
Also, probably worth checking the layout for a testcase like the following, to check the interaction between the extra padding and the nvsize:
[ ... ]
Thanks; done above. The padding does not occur within the nvsize. I guess you also mean that we should check this against the new implementation as well.

_______________________________________________
cfe-dev mailing list
[hidden email]
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-dev
Reply | Threaded
Open this post in threaded view
|

Re: [RFC] Adding AIX power alignment rule in clang front end

Fangrui Song via cfe-dev
On Sat, Apr 25, 2020 at 10:01 PM Hubert Tong via cfe-dev <[hidden email]> wrote:
On Fri, Apr 24, 2020 at 3:55 PM Eli Friedman via cfe-dev <[hidden email]> wrote:

Assuming I’m understanding the rule correctly, it doesn’t actually affect “alignment” of the type in the sense we would normally understand it.  A struct with a double as the first member can be stored at an address with four-byte alignment.  The rule only involves inserting extra padding at the end of the struct. In particular, from the way you’re describing the rule, I think the special alignment rule isn’t supposed to affect the result of _Alignof.  Does that seem right?

The reference implementations on AIX indicate that the result of _Alignof is affected.

C:
typedef struct A { double x; int y; } A;
extern char x[_Alignof(A)];
extern char x[8];

C++:
struct A {
  A(const A &);
  double x; int y;
};
struct B : A { int z; };

extern char x[sizeof(B)];
extern char x[16];

extern char y[alignof(A)];
extern char y[8];

Unfortunately, this is invalid behavior. _Alignof/alignof must return the required alignment for a type. Given that per your previous example:
struct C {
  int n;
  struct A a;
};
would place the type A at an offset of 4, alignof(A) cannot correctly return greater than 4.

You also need to ensure that the alignment values that Clang specifies when creating memory operations in LLVM IR are correct, and not higher than they should be, which they will be, if you've set the alignment of the structure too high.

I believe the best option would be to modify the struct layout code to round up the size as Eli suggests, and then modify ASTContext::getPreferredTypeAlign so that variables of these types get an increased alignment. This function is also used to implement the non-standard GCC extension "__alignof__(T)", which unlike alignof/_Alignof, returns the "preferred" alignment of T, rather than the minimum alignment.

(BTW, the name "Preferred alignment" in this context is somewhat of a misnomer. It makes it sound like this "preferred" alignment is not part of the ABI. But it actually is, as implemented in clang today, because it's used as the assumed alignment of an extern (or common or comdat) global variable of that type, not just when defining a variable. I suspect Clang doesn't actually get this correct for some of the other non-i386 ABIs which have underaligned doubles.)

Given that, do you actually need to store anything in the RecordLayout at all?

Does the new information change your analysis or it is still worthwhile to internally treat the alignment as the lower value (and add special code to introduce the padding-for-preferred-alignment)?
 
Also, probably worth checking the layout for a testcase like the following, to check the interaction between the extra padding and the nvsize:
[ ... ]
Thanks; done above. The padding does not occur within the nvsize. I guess you also mean that we should check this against the new implementation as well.
_______________________________________________
cfe-dev mailing list
[hidden email]
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-dev

_______________________________________________
cfe-dev mailing list
[hidden email]
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-dev
Reply | Threaded
Open this post in threaded view
|

Re: [RFC] Adding AIX power alignment rule in clang front end

Fangrui Song via cfe-dev
On Sun, Apr 26, 2020 at 10:23 AM James Y Knight <[hidden email]> wrote:
Unfortunately, this is invalid behavior. _Alignof/alignof must return the required alignment for a type. Given that per your previous example:
struct C {
  int n;
  struct A a;
};
would place the type A at an offset of 4, alignof(A) cannot correctly return greater than 4.
I am inclined to agree. There are practical effects on the return value of such queries for user programs such as problems with PointerIntPair-like mechanisms and the minimally-guaranteed alignment barring contortions is the safe value. I'm not sure if we will eventually need a further extension (under option control) to make the return value match the reference implementations.
 

You also need to ensure that the alignment values that Clang specifies when creating memory operations in LLVM IR are correct, and not higher than they should be, which they will be, if you've set the alignment of the structure too high.
If a user wishes to operate on an under-aligned object with Clang, what is the recommendation? I am asking mainly in terms of the user source.
 

I believe the best option would be to modify the struct layout code to round up the size as Eli suggests, and then modify ASTContext::getPreferredTypeAlign so that variables of these types get an increased alignment. This function is also used to implement the non-standard GCC extension "__alignof__(T)", which unlike alignof/_Alignof, returns the "preferred" alignment of T, rather than the minimum alignment.
Thanks. I think we can explore that.


_______________________________________________
cfe-dev mailing list
[hidden email]
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-dev
Reply | Threaded
Open this post in threaded view
|

Re: [RFC] Adding AIX power alignment rule in clang front end

Fangrui Song via cfe-dev
In reply to this post by Fangrui Song via cfe-dev
Hi,
Thank you for all of your involvement and suggestion. Based on our discussion in this thread, a new patch has been posted on the Phabricator: https://reviews.llvm.org/D79719.

Regards,
Xiangling

On Fri, Apr 24, 2020 at 9:20 AM Xiangling Liao <[hidden email]> wrote:

Currently we are trying to implement AIX special alignment rule on the top of Itanium ABI in Clang. On AIX,  in aggregates, the first member of double/long double is aligned according to its natural alignment value; subsequent double/long double of the aggregate are aligned on 4-byte boundaries.

 

This rule is recursive. It applies all the way to the inner most nested members. An example is as the following:

 

struct D {
        double d;
        int i;
};

 

struct IntFirst {
        int i;
        struct D d;
};

 

The size of struct D is 16, class alignment is 8;

The size of struct IntFirst is 20, class alignment is 4;

 

So our current idea is to have an alignment field to record nested struct info and pass the special alignment value all the way back to the ourter most aggregate.

 

And based on this idea, there are two possible ways.

 

1.

The first one is that we introduced a new field in class ASTRecordLayout

Called `AIXOffsetAlignment`  which represents the special AIX alignment for the object that

contains floating-point member or sub-member. This is for AIX-ABI only.

 

The proposed change has been put on Phabricator under review: https://reviews.llvm.org/D78563

 

Pros:

- The design is cleaner. It is clear that such field is for AIX and it does not affect any other target

   in terms of functionality.



Cons:

- All other targets using ItaniumRecordLayoutBuilder will also have to pay the price to keep

   updating this `AIXOffsetAlignment` along the common code path by using `UpdateAlignment`.

 

2.

The second possible way we have in mind is to overload the usage of current Microsoft-ABI only alignment field `RequiredAlignment` and rename it to `TargetSpecialAlignment` instead of the above first way to create a new alignment field.

This alignment field will function the same in Itanium ABI part for AIX. Meanwhile, because the current `RequiredAlignment` is only used in MS code, renaming will not affect MS code path functionally.

 

Pro:

- Class ASTRecordLayout does not need to construct one more field `AIXOffsetAlignment` compared to the method one.

- Instead of having each target add new fields for their special alignment rules, we could have one field that handles all the potential alignment differences for every target.

 

Cons:

-  Still, all other targets using ItaniumRecordLayoutBuilder will also have to pay the price to   

   keep updating this `TargetSpecialAlignment` along the common code path by using

   `UpdateAlignment`.

-  By overloading the usage, it may create confusion when more targets try to utilize it in the

   future.

-  The degree of willingness of sharing this current MS-only alignment field is unknown.

 

 

I would like to see how people think about the above two ways. Which one is better? Any concerns about either one? Or any other ideas to suggest?

 

Please let me know if there are any questions about my current proposal and design. Your feedback is appreciated.

 

Regards,

 

Xiangling Liao


_______________________________________________
cfe-dev mailing list
[hidden email]
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-dev