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

Mon Jul 8 15:08:45 PDT 2013

i''ll spend a wee bit of time doing the same fixup to the other two test
examples later today or tomorrow. Thanks for walking me through this!

On Mon, Jul 8, 2013 at 6:06 PM, Carter Schonwald <carter.schonwald at gmail.com
> wrote:

> looks like I just fixed it up  to match your patch as a learning exercise,
> test passes now.
>
>
> On Mon, Jul 8, 2013 at 6:03 PM, Stephen Lin <swlin at post.harvard.edu>wrote:
>
>> Yes, I ran the test through "make" in the test directory.
>>
>> You can also do:
>>
>>    llc -tailcallopt -mtriple=x86_64-linux-gnu -mcpu=core-avx2 -o - <
>> test/CodeGen/X86/ghc-cc64-avx.ll
>>
>> to see the output directly.
>>
>> Are you sure you applied the patch I sent last cleanly against trunk?
>> I just did an SVN diff again, so I'll attach the patch again (I think
>> it's the same as what I just sent, though.)
>>
>> Stephen
>>
>> On Mon, Jul 8, 2013 at 2:55 PM, Carter Schonwald
>> <carter.schonwald at gmail.com> wrote:
>> > did you test the modified patch on your side? Because adding the
>> volatiles
>> > makes the test fail!
>> >
>> > the error is
>> >
>> > ********************
>> > FAIL: LLVM :: CodeGen/X86/ghc-cc64-avx.ll (3691 of 8368)
>> > ******************** TEST 'LLVM :: CodeGen/X86/ghc-cc64-avx.ll' FAILED
>> > ********************
>> > Script:
>> > --
>> > /Users/carter/Desktop/repoScratcher/llvm-thetool/Debug+Asserts/bin/llc <
>> >
>> /Users/carter/Desktop/repoScratcher/llvm-thetool/test/CodeGen/X86/ghc-cc64-avx.ll
>> > -tailcallopt -mtriple=x86_64-linux-gnu -mcpu=core-avx2  |
>> >
>> /Users/carter/Desktop/repoScratcher/llvm-thetool/Debug+Asserts/bin/FileCheck
>> >
>> /Users/carter/Desktop/repoScratcher/llvm-thetool/test/CodeGen/X86/ghc-cc64-avx.ll
>> > --
>> > Exit Code: 1
>> > Command Output (stderr):
>> > --
>> >
>> /Users/carter/Desktop/repoScratcher/llvm-thetool/test/CodeGen/X86/ghc-cc64-avx.ll:45:11:
>> > error: expected string not found in input
>> >  ; CHECK: movq sp(%rip), %rbp
>> >           ^
>> > <stdin>:63:2: note: scanning from here
>> >  popq %rax
>> >  ^
>> >
>> > Looking at the test code, I think i see what happens, i'll give you a
>> patch
>> > that works around that prolbme
>> >
>> >
>> >
>> >
>> > On Mon, Jul 8, 2013 at 5:49 PM, Stephen Lin <swlin at post.harvard.edu>
>> wrote:
>> >>
>> >> 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 --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20130708/0f83c14b/attachment.html>