<div dir="ltr">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!</div><div class="gmail_extra"><br><br><div class="gmail_quote">
On Mon, Jul 8, 2013 at 6:06 PM, Carter Schonwald <span dir="ltr"><<a href="mailto:carter.schonwald@gmail.com" target="_blank">carter.schonwald@gmail.com</a>></span> wrote:<br><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
<div dir="ltr">looks like I just fixed it up to match your patch as a learning exercise, test passes now.</div><div class="HOEnZb"><div class="h5"><div class="gmail_extra"><br><br><div class="gmail_quote">On Mon, Jul 8, 2013 at 6:03 PM, Stephen Lin <span dir="ltr"><<a href="mailto:swlin@post.harvard.edu" target="_blank">swlin@post.harvard.edu</a>></span> wrote:<br>
<blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">Yes, I ran the test through "make" in the test directory.<br>
<br>
You can also do:<br>
<br>
llc -tailcallopt -mtriple=x86_64-linux-gnu -mcpu=core-avx2 -o - <<br>
test/CodeGen/X86/ghc-cc64-avx.ll<br>
<br>
to see the output directly.<br>
<br>
Are you sure you applied the patch I sent last cleanly against trunk?<br>
I just did an SVN diff again, so I'll attach the patch again (I think<br>
it's the same as what I just sent, though.)<br>
<br>
Stephen<br>
<br>
On Mon, Jul 8, 2013 at 2:55 PM, Carter Schonwald<br>
<div><div><<a href="mailto:carter.schonwald@gmail.com" target="_blank">carter.schonwald@gmail.com</a>> wrote:<br>
> did you test the modified patch on your side? Because adding the volatiles<br>
> makes the test fail!<br>
><br>
> the error is<br>
><br>
> ********************<br>
> FAIL: LLVM :: CodeGen/X86/ghc-cc64-avx.ll (3691 of 8368)<br>
> ******************** TEST 'LLVM :: CodeGen/X86/ghc-cc64-avx.ll' FAILED<br>
> ********************<br>
> Script:<br>
> --<br>
> /Users/carter/Desktop/repoScratcher/llvm-thetool/Debug+Asserts/bin/llc <<br>
> /Users/carter/Desktop/repoScratcher/llvm-thetool/test/CodeGen/X86/ghc-cc64-avx.ll<br>
> -tailcallopt -mtriple=x86_64-linux-gnu -mcpu=core-avx2 |<br>
> /Users/carter/Desktop/repoScratcher/llvm-thetool/Debug+Asserts/bin/FileCheck<br>
> /Users/carter/Desktop/repoScratcher/llvm-thetool/test/CodeGen/X86/ghc-cc64-avx.ll<br>
> --<br>
> Exit Code: 1<br>
> Command Output (stderr):<br>
> --<br>
> /Users/carter/Desktop/repoScratcher/llvm-thetool/test/CodeGen/X86/ghc-cc64-avx.ll:45:11:<br>
> error: expected string not found in input<br>
> ; CHECK: movq sp(%rip), %rbp<br>
> ^<br>
> <stdin>:63:2: note: scanning from here<br>
> popq %rax<br>
> ^<br>
><br>
> Looking at the test code, I think i see what happens, i'll give you a patch<br>
> that works around that prolbme<br>
><br>
><br>
><br>
><br>
> On Mon, Jul 8, 2013 at 5:49 PM, Stephen Lin <<a href="mailto:swlin@post.harvard.edu" target="_blank">swlin@post.harvard.edu</a>> wrote:<br>
>><br>
>> No, I think you read that wrong or I was unclear. CHECK-NEXT means<br>
>> that a line must come immediately after the previous match, with no<br>
>> lines in between. It is more stringent than CHECK, which just means<br>
>> that the line to match must come at some point after the previous<br>
>> CHECK. Please read the docs at<br>
>> <a href="http://llvm.org/docs/CommandGuide/FileCheck.html" target="_blank">http://llvm.org/docs/CommandGuide/FileCheck.html</a> .<br>
>><br>
>> Changing CHECK to CHECK-NEXT will often cause failures; the reverse<br>
>> should never happen, since the latter matches a strict subset of what<br>
>> the former matches.<br>
>><br>
>> Anyway, I updated your patch to use volatile. Basically, once things<br>
>> are volatile, they are loaded/stored in the order they appear in the<br>
>> IR, so I had to reorder things to get the patch to work.<br>
>><br>
>> Your existing patch depends on the compiler executing the loads in a<br>
>> specific order for the test to succeed, but there is nothing stopping<br>
>> the instruction scheduler from changing the order of the instructions<br>
>> and breaking the test. This is a maintenance hassle, because it means<br>
>> that whoever "broke" your test has to figure out if the break is real<br>
>> one or not by trying to figure out the purpose of the test (which<br>
>> might requiring them to learn the nitty details of the GHC calling<br>
>> convention, if it happens to be this test.)<br>
>><br>
>> Please update the other GHC calling convention tests in the same way,<br>
>> as a separate patch.<br>
>><br>
>> Stephen<br>
>><br>
>> On Mon, Jul 8, 2013 at 2:40 PM, Carter Schonwald<br>
>> <<a href="mailto:carter.schonwald@gmail.com" target="_blank">carter.schonwald@gmail.com</a>> wrote:<br>
>> > in some of the test examples, if I replace CHECK with CHECK-NEXT, the<br>
>> > tests<br>
>> > fail..., which suggests to me that this isn't quite true!<br>
>> ><br>
>> ><br>
>> > On Mon, Jul 8, 2013 at 5:20 PM, Stephen Lin <<a href="mailto:swlin@post.harvard.edu" target="_blank">swlin@post.harvard.edu</a>><br>
>> > wrote:<br>
>> >><br>
>> >> As I understand it, volatile just means that the load will not be<br>
>> >> reordered with respect to other volatile loads or stores, so I don't<br>
>> >> know why it would break the test. I'll try it locally.<br>
>> >><br>
>> >> Changing CHECK-NEXT to CHECK just means you are not longer depending<br>
>> >> on the lines being immediately after each other, but you are still<br>
>> >> depending on the loads being scheduled in the order you specified.<br>
>> >><br>
>> >> Stephen<br>
>> >><br>
>> >> On Mon, Jul 8, 2013 at 2:12 PM, Carter Schonwald<br>
>> >> <<a href="mailto:carter.schonwald@gmail.com" target="_blank">carter.schonwald@gmail.com</a>> wrote:<br>
>> >> > hrmm, I think those loads should not be volatile, because the<br>
>> >> > semantics<br>
>> >> > of<br>
>> >> > ghc means that function args are "immutable".<br>
>> >> ><br>
>> >> > (tellingly, the test example fails with the volatile annotation and<br>
>> >> > CHECK<br>
>> >> > instead of CHECK-NEXT, but works just fine with CHECK and no<br>
>> >> > volatile)<br>
>> >> ><br>
>> >> > attached is the update of the proposed patch, passes the test suite<br>
>> >> > again,<br>
>> >> > i'll email the list with a separate patch for those test examples<br>
>> >> > shortly<br>
>> >> ><br>
>> >> ><br>
>> >> > On Mon, Jul 8, 2013 at 5:05 PM, Stephen Lin <<a href="mailto:swlin@post.harvard.edu" target="_blank">swlin@post.harvard.edu</a>><br>
>> >> > wrote:<br>
>> >> >><br>
>> >> >> It's not that big of a deal, I think, but it's probably better to<br>
>> >> >> change them if you can. People often introduce changes that cause<br>
>> >> >> existing tests to fail just because the tests are over-specified and<br>
>> >> >> it takes time for the person making the change to figure out if the<br>
>> >> >> change actually breaks something important or not.<br>
>> >> >><br>
>> >> >> You should probably split the cleanup of the exiting tests and the<br>
>> >> >> new<br>
>> >> >> changes into two separate patch files (one for the changes to the<br>
>> >> >> existing tests, and one for the new stuff), since they're disjoint.<br>
>> >> >><br>
>> >> >> -Stephen<br>
>> >> >><br>
>> >> >> On Mon, Jul 8, 2013 at 1:56 PM, Carter Schonwald<br>
>> >> >> <<a href="mailto:carter.schonwald@gmail.com" target="_blank">carter.schonwald@gmail.com</a>> wrote:<br>
>> >> >> > Ok, will do,<br>
>> >> >> > I should remark that the current tests for vanil x86_32 and x86_64<br>
>> >> >> > ghc<br>
>> >> >> > calling conventions are using check-next currently, so i was<br>
>> >> >> > copying<br>
>> >> >> > that<br>
>> >> >> > convention. Should I update the patch to change those to check<br>
>> >> >> > too?<br>
>> >> >> ><br>
>> >> >> ><br>
>> >> >> > On Mon, Jul 8, 2013 at 4:54 PM, Stephen Lin<br>
>> >> >> > <<a href="mailto:swlin@post.harvard.edu" target="_blank">swlin@post.harvard.edu</a>><br>
>> >> >> > wrote:<br>
>> >> >> >><br>
>> >> >> >> Hi Carter,<br>
>> >> >> >><br>
>> >> >> >> I'm not sure if you can really depend on loads being ordered in<br>
>> >> >> >> the<br>
>> >> >> >> order you specify them in the IR; even if they are now, they<br>
>> >> >> >> might<br>
>> >> >> >> not<br>
>> >> >> >> be in the future and it will add to the difficulty of testing<br>
>> >> >> >> changes<br>
>> >> >> >> later, so it's better not to specify things you don't actually<br>
>> >> >> >> depend<br>
>> >> >> >> upon.<br>
>> >> >> >><br>
>> >> >> >> Can you change the loads to volatile loads (just put the keyword<br>
>> >> >> >> "volatile" after the load) and change the CHECK-NEXT lines to<br>
>> >> >> >> simply<br>
>> >> >> >> CHECK? I haven't tried it yet but think that would work and be<br>
>> >> >> >> more<br>
>> >> >> >> robust to future changes.<br>
>> >> >> >><br>
>> >> >> >> Thanks,<br>
>> >> >> >> Stephen<br>
>> >> >> >><br>
>> >> >> >> On Mon, Jul 8, 2013 at 1:37 PM, Carter Tazio Schonwald<br>
>> >> >> >> <<a href="mailto:carter.schonwald@gmail.com" target="_blank">carter.schonwald@gmail.com</a>> wrote:<br>
>> >> >> >> > Hey All,<br>
>> >> >> >> ><br>
>> >> >> >> > currently the GHC calling convention doesnt have support for<br>
>> >> >> >> > using<br>
>> >> >> >> > the<br>
>> >> >> >> > AVX<br>
>> >> >> >> > 256bit width registers. Attached is a patch that augments the<br>
>> >> >> >> > GHC<br>
>> >> >> >> > x86-64<br>
>> >> >> >> > calling convention with that support when vector types of that<br>
>> >> >> >> > size<br>
>> >> >> >> > are<br>
>> >> >> >> > used.<br>
>> >> >> >> ><br>
>> >> >> >> > This change has been Ok'd by the GHC HQ devs responsible for<br>
>> >> >> >> > the<br>
>> >> >> >> > SIMD<br>
>> >> >> >> > support recently added to ghc (as well as the principal author<br>
>> >> >> >> > of<br>
>> >> >> >> > the<br>
>> >> >> >> > llvm<br>
>> >> >> >> > backend for ghc ). see the ghc ticket here<br>
>> >> >> >> > <a href="http://hackage.haskell.org/trac/ghc/ticket/8033" target="_blank">http://hackage.haskell.org/trac/ghc/ticket/8033</a> for their<br>
>> >> >> >> > indications<br>
>> >> >> >> > of<br>
>> >> >> >> > approval<br>
>> >> >> >> ><br>
>> >> >> >> > it'd be really great to have this patch in both the 3.4 and the<br>
>> >> >> >> > pending<br>
>> >> >> >> > 3.3<br>
>> >> >> >> > point release, because then the next GHC release could get some<br>
>> >> >> >> > additional<br>
>> >> >> >> > work to support AVX2 now rather than later. (in addition to<br>
>> >> >> >> > the<br>
>> >> >> >> > current<br>
>> >> >> >> > support for 128bit simd)<br>
>> >> >> >> ><br>
>> >> >> >> > i've included the patch and an additional test case in the diff<br>
>> >> >> >> > attached<br>
>> >> >> >> > below<br>
>> >> >> >> ><br>
>> >> >> >> > thanks!<br>
>> >> >> >> > -Carter<br>
>> >> >> >> ><br>
>> >> >> >> > _______________________________________________<br>
>> >> >> >> > llvm-commits mailing list<br>
>> >> >> >> > <a href="mailto:llvm-commits@cs.uiuc.edu" target="_blank">llvm-commits@cs.uiuc.edu</a><br>
>> >> >> >> > <a href="http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits" target="_blank">http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits</a><br>
>> >> >> >> ><br>
>> >> >> ><br>
>> >> >> ><br>
>> >> ><br>
>> >> ><br>
>> ><br>
>> ><br>
><br>
><br>
</div></div></blockquote></div><br></div>
</div></div></blockquote></div><br></div>