[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:06:50 PDT 2013

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/57d3769b/attachment.html>