[PATCH ] adding AVX 256bit register support to ghc_x86-64 calling convention (hopefully for both 3.4 and 3.3 point release)

[Stephen Lin]

No, I think you read that wrong or I was unclear. CHECK-NEXT means
that a line must come immediately after the previous match, with no
lines in between. It is more stringent than CHECK, which just means
that the line to match must come at some point after the previous
CHECK. Please read the docs at
http://llvm.org/docs/CommandGuide/FileCheck.html .

Changing CHECK to CHECK-NEXT will often cause failures; the reverse
should never happen, since the latter matches a strict subset of what
the former matches.

Anyway, I updated your patch to use volatile. Basically, once things
are volatile, they are loaded/stored in the order they appear in the
IR, so I had to reorder things to get the patch to work.

Your existing patch depends on the compiler executing the loads in a
specific order for the test to succeed, but there is nothing stopping
the instruction scheduler from changing the order of the instructions
and breaking the test. This is a maintenance hassle, because it means
that whoever "broke" your test has to figure out if the break is real
one or not by trying to figure out the purpose of the test (which
might requiring them to learn the nitty details of the GHC calling
convention, if it happens to be this test.)

Please update the other GHC calling convention tests in the same way,
as a separate patch.

Stephen

On Mon, Jul 8, 2013 at 2:40 PM, Carter Schonwald
<carter.schonwald at gmail.com> wrote:
> in some of the test examples, if I replace CHECK with CHECK-NEXT, the tests
> fail..., which suggests to me that this isn't quite true!
>
>
> On Mon, Jul 8, 2013 at 5:20 PM, Stephen Lin <swlin at post.harvard.edu> wrote:
>>
>> As I understand it, volatile just means that the load will not be
>> reordered with respect to other volatile loads or stores, so I don't
>> know why it would break the test. I'll try it locally.
>>
>> Changing CHECK-NEXT to CHECK just means you are not longer depending
>> on the lines being immediately after each other, but you are still
>> depending on the loads being scheduled in the order you specified.
>>
>> Stephen
>>
>> On Mon, Jul 8, 2013 at 2:12 PM, Carter Schonwald
>> <carter.schonwald at gmail.com> wrote:
>> > hrmm, I think those loads should not be volatile, because the semantics
>> > of
>> > ghc means that function args are "immutable".
>> >
>> > (tellingly, the test example fails with the volatile annotation and
>> > CHECK
>> > instead of CHECK-NEXT, but works just fine with CHECK and no volatile)
>> >
>> > attached is the update of the proposed patch, passes the test suite
>> > again,
>> > i'll email the list with a separate patch for those test examples
>> > shortly
>> >
>> >
>> > On Mon, Jul 8, 2013 at 5:05 PM, Stephen Lin <swlin at post.harvard.edu>
>> > wrote:
>> >>
>> >> It's not that big of a deal, I think, but it's probably better to
>> >> change them if you can. People often introduce changes that cause
>> >> existing tests to fail just because the tests are over-specified and
>> >> it takes time for the person making the change to figure out if the
>> >> change actually breaks something important or not.
>> >>
>> >> You should probably split the cleanup of the exiting tests and the new
>> >> changes into two separate patch files (one for the changes to the
>> >> existing tests, and one for the new stuff), since they're disjoint.
>> >>
>> >> -Stephen
>> >>
>> >> On Mon, Jul 8, 2013 at 1:56 PM, Carter Schonwald
>> >> <carter.schonwald at gmail.com> wrote:
>> >> > Ok, will do,
>> >> > I should remark that the current tests for vanil x86_32 and x86_64
>> >> > ghc
>> >> > calling conventions are using check-next currently, so i was copying
>> >> > that
>> >> > convention. Should I update the patch to change those to check too?
>> >> >
>> >> >
>> >> > On Mon, Jul 8, 2013 at 4:54 PM, Stephen Lin <swlin at post.harvard.edu>
>> >> > wrote:
>> >> >>
>> >> >> Hi Carter,
>> >> >>
>> >> >> I'm not sure if you can really depend on loads being ordered in the
>> >> >> order you specify them in the IR; even if they are now, they might
>> >> >> not
>> >> >> be in the future and it will add to the difficulty of testing
>> >> >> changes
>> >> >> later, so it's better not to specify things you don't actually
>> >> >> depend
>> >> >> upon.
>> >> >>
>> >> >> Can you change the loads to volatile loads (just put the keyword
>> >> >> "volatile" after the load) and change the CHECK-NEXT lines to simply
>> >> >> CHECK? I haven't tried it yet but think that would work and be more
>> >> >> robust to future changes.
>> >> >>
>> >> >> Thanks,
>> >> >> Stephen
>> >> >>
>> >> >> On Mon, Jul 8, 2013 at 1:37 PM, Carter Tazio Schonwald
>> >> >> <carter.schonwald at gmail.com> wrote:
>> >> >> > Hey All,
>> >> >> >
>> >> >> > currently the GHC calling convention doesnt have support for using
>> >> >> > the
>> >> >> > AVX
>> >> >> > 256bit width registers. Attached is a patch that augments the GHC
>> >> >> > x86-64
>> >> >> > calling convention with that support when vector types of that
>> >> >> > size
>> >> >> > are
>> >> >> > used.
>> >> >> >
>> >> >> > This change has been Ok'd by the GHC HQ devs responsible for the
>> >> >> > SIMD
>> >> >> > support recently added to ghc (as well as the principal author of
>> >> >> > the
>> >> >> > llvm
>> >> >> > backend for ghc ).  see the ghc ticket here
>> >> >> > http://hackage.haskell.org/trac/ghc/ticket/8033 for their
>> >> >> > indications
>> >> >> > of
>> >> >> > approval
>> >> >> >
>> >> >> > it'd be really great to have this patch in both the 3.4 and the
>> >> >> > pending
>> >> >> > 3.3
>> >> >> > point release, because then the next GHC release could get some
>> >> >> > additional
>> >> >> > work to support AVX2   now rather than later. (in addition to the
>> >> >> > current
>> >> >> > support for 128bit simd)
>> >> >> >
>> >> >> > i've included the patch and an additional test case in the diff
>> >> >> > attached
>> >> >> > below
>> >> >> >
>> >> >> > thanks!
>> >> >> > -Carter
>> >> >> >
>> >> >> > _______________________________________________
>> >> >> > llvm-commits mailing list
>> >> >> > llvm-commits at cs.uiuc.edu
>> >> >> > http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits
>> >> >> >
>> >> >
>> >> >
>> >
>> >
>
>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: ghc-x86-64-avx-volatile.patch
Type: application/octet-stream
Size: 4463 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20130708/7662cad4/attachment.obj>