PNaCl: Enable ExpandByVal pass for expanding out passing structs by value

PNaCl: Enable ExpandByVal pass for expanding out passing structs by value

We have to disable two calling conventions tests which test
__attribute__((aligned)) structs.  pnacl-clang does not align these
properly when they are function arguments: it omits the LLVM "align"
argument attribute, but since it generates aligned memory accesses,
these become "movaps" on x86 and they fault.  I think these tests were
passing by luck before, because the stack location happened to be
16-byte-aligned.

This change will require a change to the PNaCl PPAPI shims when the
PNaCl toolchain is rolled into Chromium.

BUG=https://code.google.com/p/nativeclient/issues/detail?id=3400
BUG=https://code.google.com/p/nativeclient/issues/detail?id=3403
TEST=PNaCl toolchain trybots + GCC torture tests + LLVM test suite + Spec2k

Committed: https://src.chromium.org/viewvc/native_client?view=rev&revision=11232

[jvoung (off chromium), 2013-04-24 19:59:12]

https://codereview.chromium.org/14317008/diff/17001/pnacl/driver/pnacl-ld.py
File pnacl/driver/pnacl-ld.py (right):

https://codereview.chromium.org/14317008/diff/17001/pnacl/driver/pnacl-ld.py#...
pnacl/driver/pnacl-ld.py:361: '-expand-byval',
When you did the byval -> reference pass did you add any noalias hints (was it
valid to do so)?

I ask because the ArgumentPromotionPass (which may run after this, at -O3) can
simplify byval arguments for internal functions.  I'm not sure it would do as
much with just a raw pointer.  It does try to simplify pointer arguments too,
given that the alias analysis says that it is valid to do so.

[Mark Seaborn, 2013-04-24 20:36:42]

https://codereview.chromium.org/14317008/diff/17001/pnacl/driver/pnacl-ld.py
File pnacl/driver/pnacl-ld.py (right):

https://codereview.chromium.org/14317008/diff/17001/pnacl/driver/pnacl-ld.py#...
pnacl/driver/pnacl-ld.py:361: '-expand-byval',
On 2013/04/24 19:59:12, jvoung (cr) wrote:
> When you did the byval -> reference pass did you add any noalias hints

No, I didn't.  (See https://codereview.chromium.org/13973018/)

> (was it valid to do so)?

It seems that way from LangRef, although I've got no experience with alias
analyses (aside from debugging that "tail" attribute problem this week).  I'd
want to be sure I'm not risking a miscompilation before adding a noalias attr.
:-)

> I ask because the ArgumentPromotionPass (which may run after this, at -O3) can
> simplify byval arguments for internal functions.  I'm not sure it would do as
> much with just a raw pointer.  It does try to simplify pointer arguments too,
> given that the alias analysis says that it is valid to do so.

So, should I move -expand-byval to the second set of ABI-simplification passes,
which run after -std-link-opts?

[Mark Seaborn, 2013-04-24 20:44:07]

BTW, I tried disabling those tests by doing:

#if !defined(__clang__)
DO_FOR_TYPE(I32_CHAR_ALIGN32)
#endif

(so that they would still run with gcc)

But "./scons run_call_structs_test platform=arm" gave the error:

scons-out/nacl-arm-pnacl-clang/obj/tests/callingconv_case_by_case/call_structs.2.o:
error: undefined reference to 'mod3_I32_CHAR_ALIGN32(I32_CHAR_ALIGN32)'

I can't explain that failure, but debugging it didn't seem very worthwhile, so I
disabled the two tests unconditionally.

[jvoung (off chromium), 2013-04-24 21:43:09]

lgtm

https://codereview.chromium.org/14317008/diff/17001/pnacl/driver/pnacl-ld.py
File pnacl/driver/pnacl-ld.py (right):

https://codereview.chromium.org/14317008/diff/17001/pnacl/driver/pnacl-ld.py#...
pnacl/driver/pnacl-ld.py:361: '-expand-byval',
On 2013/04/24 20:36:42, Mark Seaborn wrote:
> On 2013/04/24 19:59:12, jvoung (cr) wrote:
> > When you did the byval -> reference pass did you add any noalias hints
> 
> No, I didn't.  (See https://codereview.chromium.org/13973018/)
> 
> > (was it valid to do so)?
> 
> It seems that way from LangRef, although I've got no experience with alias
> analyses (aside from debugging that "tail" attribute problem this week).  I'd
> want to be sure I'm not risking a miscompilation before adding a noalias attr.
> :-)
> 
> > I ask because the ArgumentPromotionPass (which may run after this, at -O3)
can
> > simplify byval arguments for internal functions.  I'm not sure it would do
as
> > much with just a raw pointer.  It does try to simplify pointer arguments
too,
> > given that the alias analysis says that it is valid to do so.
> 
> So, should I move -expand-byval to the second set of ABI-simplification
passes,
> which run after -std-link-opts?

Well, I'm okay with leaving it here, and filing an issue to investigate noalias
or changing the order.  It might not actually matter much =/

[jvoung (off chromium), 2013-04-24 21:46:57]

On 2013/04/24 20:44:07, Mark Seaborn wrote:
> BTW, I tried disabling those tests by doing:
> 
> #if !defined(__clang__)
> DO_FOR_TYPE(I32_CHAR_ALIGN32)
> #endif
> 
> (so that they would still run with gcc)
> 
> But "./scons run_call_structs_test platform=arm" gave the error:
> 
>
scons-out/nacl-arm-pnacl-clang/obj/tests/callingconv_case_by_case/call_structs.2.o:
> error: undefined reference to 'mod3_I32_CHAR_ALIGN32(I32_CHAR_ALIGN32)'
> 
> I can't explain that failure, but debugging it didn't seem very worthwhile, so
I
> disabled the two tests unconditionally.

There's a case where all the files are built by gcc, or all clang, and there's a
case where half of the .o files are built by gcc and the other half are built by
clang, so it probably won't work to just check !defined(__clang__).

[Mark Seaborn, 2013-04-25 18:16:39]

Committed patchset #3 manually as r11232 (presubmit successful).

[Mark Seaborn, 2013-04-25 18:39:51]

On 24 April 2013 14:43, <jvoung@chromium.org> wrote:

>  > I ask because the ArgumentPromotionPass (which may run after this, at
>> -O3) can
>>
> > simplify byval arguments for internal functions.  I'm not sure it would
>> do as
>>
> > much with just a raw pointer.  It does try to simplify pointer arguments
>> too,
>>
> > given that the alias analysis says that it is valid to do so.
>>
>
>  So, should I move -expand-byval to the second set of ABI-simplification
>> passes,
>>
> which run after -std-link-opts?
>>
>
> Well, I'm okay with leaving it here, and filing an issue to investigate
> noalias or changing the order.  It might not actually matter much =/
>

I've filed https://code.google.com/p/nativeclient/issues/detail?id=3408.

It looks like reordering the passes would give more benefit than adding
"noalias".  (Though to get more benefit we could run optimisation passes
before and after ABI simplifications.)

It looks like "byval" implies that the argument doesn't alias.  For
example, the dead store in
  void foo(struct s x) { x.y = 1; }
gets removed, whereas it doesn't after "-expand-byval", even if I add a
"noalias" attribute (as you'd expect).

Mark

-- 
You received this message because you are subscribed to the Google Groups
"Native-Client-Reviews" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to native-client-reviews+unsubscribe@googlegroups.com.
To post to this group, send email to native-client-reviews@googlegroups.com.
Visit this group at http://groups.google.com/group/native-client-reviews?hl=en.
For more options, visit https://groups.google.com/groups/opt_out.

[Mark Seaborn, 2013-04-25 18:41:05]

On 24 April 2013 14:46, <jvoung@chromium.org> wrote:

> On 2013/04/24 20:44:07, Mark Seaborn wrote:
>
>> BTW, I tried disabling those tests by doing:
>>
>
>  #if !defined(__clang__)
>> DO_FOR_TYPE(I32_CHAR_ALIGN32)
>> #endif
>>
>
>  (so that they would still run with gcc)
>>
>
>  But "./scons run_call_structs_test platform=arm" gave the error:
>>
>>
>>
scons-out/nacl-arm-pnacl-clang/obj/tests/callingconv_case_by_case/call_structs.2.o:
>>
> error: undefined reference to 'mod3_I32_CHAR_ALIGN32(I32_**CHAR_ALIGN32)'
>>
>
>  I can't explain that failure, but debugging it didn't seem very
>> worthwhile, so I
>>
> disabled the two tests unconditionally.
>>
>
> There's a case where all the files are built by gcc, or all clang, and
> there's a
> case where half of the .o files are built by gcc and the other half are
> built by
> clang, so it probably won't work to just check !defined(__clang__).
>

Ah yes, of course.   That is the purpose of the calling conventions tests,
so I should have thought of that. :-)

Mark

-- 
You received this message because you are subscribed to the Google Groups
"Native-Client-Reviews" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to native-client-reviews+unsubscribe@googlegroups.com.
To post to this group, send email to native-client-reviews@googlegroups.com.
Visit this group at http://groups.google.com/group/native-client-reviews?hl=en.
For more options, visit https://groups.google.com/groups/opt_out.