This is an archive of the discontinued LLVM Phabricator instance.

[AIX] Update data directives for AIX assembly
ClosedPublic

Authored by daltenty on Jun 1 2020, 8:53 AM.

Details

Summary

The standard data emission directives (e.g. .short, .long) in the AIX assembler
have the unintended consequence of aligning their output to the natural byte
boundary. This cause problems because we aren't expecting behavior from the
Data*bitsDirectives, so the final alignment of data isn't correct in some cases
on AIX.

This patch updated the Data*bitsDirectives to use .vbyte pseudo-ops instead to emit the
data, since we will emit the .align directives as needed. We update the existing
testcases and add a test for emission of struct data.

Diff Detail

Event Timeline

daltenty created this revision.Jun 1 2020, 8:53 AM
Herald added a project: Restricted Project. · View Herald TranscriptJun 1 2020, 8:53 AM
daltenty edited the summary of this revision. (Show Details)Jun 1 2020, 10:43 AM
Xiangling_L added inline comments.Jun 1 2020, 1:43 PM
llvm/lib/MC/MCAsmInfoXCOFF.cpp
33

Just a question here.
I checked the XL behavior, in the following case:

struct {
  int i;
  double n;
} a[] = {
         {9, 1.5},
};

test.s:

.csect  a{RW}, 3
.long   0x00000009              # "\0\0\0\t"
.long   0x3ff80000              # "?\370\0\0"
.long   0x00000000              # "\0\0\0\0"

Under 32-bit mode, XL split a double to two .long. So I am wondering can we do the same in LLVM?

llvm/test/CodeGen/PowerPC/aix-xcoff-data.ll
29

I am wondering do you make it align to 1 due to any special reason? If not, according to AIX power alignment rule, this struct is supposed to be aligned to 4.

And also, we may want to add a more interesting case where the double is the first member of the struct, which is 8-byte aligned.
e.g.

%struct.anon = type <{ double, i32 }>
@astruct = global [1 x %struct.anon] [%struct.anon <{ double 7.000000e+00 , i32 1}>], align 8

By doing that, we can see how the padding is added using vbyte.

daltenty marked 2 inline comments as done.Jun 1 2020, 4:15 PM
daltenty added inline comments.
llvm/lib/MC/MCAsmInfoXCOFF.cpp
33

Currently we cannot (we'll split it into two .vbyte 4, the way gcc does), as .long has implicit alignment behaviour (i.e. what we are trying to avoid in this patch) and at the point of value emission we don't have enough context to tell if it's safe to use.

AFAIK the streamer is expecting alignment constraints to already be taken care of explicitly by emission of .align,etc. by the time we get to emitting values, so we can't use any of the implicitly aligned directives.

llvm/test/CodeGen/PowerPC/aix-xcoff-data.ll
29

I am wondering do you make it align to 1 due to any special reason? If not, according to AIX power alignment rule, this struct is supposed to be aligned to 4.

IIUC, we needn't be concerned with this adhering to AIX power alignment rules, we can validly generate this packed form out of clang with __attribute__ ((packed)). I had used the packed form since we were mainly interested in observing the effect of data directives (and thus any implicit alignment) used to write out the members, rather than anything from the ABI.

And also, we may want to add a more interesting case where the double is the first member of the struct, which is 8-byte aligned.
By doing that, we can see how the padding is added using vbyte.

The padding is actually emitted using .space directives, so I'm not sure if it buys us much in the context of this change, but I'll add this as a non-packed case for completeness.

llvm/lib/MC/MCAsmInfoXCOFF.cpp
32

I'm sure that "standard" is the right word for classifying the directives that happen to be similarly named to ones on some other platforms.

Suggestion:
Use .vbyte for data definition to avoid directives that apply an implicit alignment.

llvm/lib/Target/PowerPC/MCTargetDesc/PPCMCAsmInfo.cpp
63

s/on/under/;

llvm/test/CodeGen/PowerPC/aix-extern-weak.ll
54

Even if we aren't consistent on how many spaces we use for where the tabs are in the real output, let's be consistent at least on having exactly one space after the comma.

llvm/test/CodeGen/PowerPC/aix-lower-constant-pool-index.ll
51

Same comment re: spaces after the comma. Please address throughout.

llvm/test/CodeGen/PowerPC/aix-lower-jump-table.ll
102

Whichever patch, between this and D80831, that lands later is going to have to deal with the merge conflict.

llvm/test/CodeGen/PowerPC/aix-xcoff-data.ll
124

Please adjust the spacing to get the directives to align within at least the block (with indentation in relation to the label).

llvm/test/CodeGen/PowerPC/aix-xcoff-mergeable-const.ll
31

Same comment re: aligning the lines. Please fix throughout.

daltenty marked 5 inline comments as done.Jun 2 2020, 8:28 AM
daltenty updated this revision to Diff 267897.Jun 2 2020, 8:30 AM
  • Update comments
  • Add unpacked struct case and add tests
  • Fix asm whitespace and block spacing in tests
jasonliu added inline comments.Jun 2 2020, 12:53 PM
llvm/lib/MC/MCAsmInfoXCOFF.cpp
23

I think the reason this was here is that we want to keep the same order as it is in the base class MCAsmInfo.
Is that something we still want to do? (which means Data16bitsDirective and Data32bitsDirective needs to go here?)

daltenty marked an inline comment as done.Jun 2 2020, 1:39 PM
daltenty added inline comments.
llvm/lib/MC/MCAsmInfoXCOFF.cpp
23

I think it makes sense to move it here

daltenty updated this revision to Diff 267976.Jun 2 2020, 1:42 PM
  • Move directives in AsmInfo into base class order
jasonliu accepted this revision.Jun 2 2020, 1:51 PM

Thanks. LGTM.

This revision is now accepted and ready to land.Jun 2 2020, 1:51 PM

LGTM with minor nit.

llvm/test/CodeGen/PowerPC/aix-xcoff-rodata.ll
31–33

Adjust spacing here.

45–47

Same here and in other places within this file.

daltenty updated this revision to Diff 268189.Jun 3 2020, 7:13 AM
  • Fix directive spacing
This revision was automatically updated to reflect the committed changes.