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

Carter Schonwald carter.schonwald at gmail.com
Mon Jul 8 14:55:14 PDT 2013


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


More information about the llvm-commits mailing list