<div dir="ltr">did you test the modified patch on your side? Because adding the volatiles makes the test fail!<div><br></div><div style>the error is </div><div style><br></div><div style><div>********************</div><div>
FAIL: LLVM :: CodeGen/X86/ghc-cc64-avx.ll (3691 of 8368)</div><div>******************** TEST 'LLVM :: CodeGen/X86/ghc-cc64-avx.ll' FAILED ********************</div><div>Script:</div><div>--</div><div>/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</div>
<div>--</div><div>Exit Code: 1</div><div>Command Output (stderr):</div><div>--</div><div>/Users/carter/Desktop/repoScratcher/llvm-thetool/test/CodeGen/X86/ghc-cc64-avx.ll:45:11: error: expected string not found in input</div>
<div> ; CHECK: movq sp(%rip), %rbp</div><div> ^</div><div><stdin>:63:2: note: scanning from here</div><div> popq %rax</div><div> ^</div><div><br></div><div style>Looking at the test code, I think i see what happens, i'll give you a patch that works around that prolbme</div>
<div><br></div><div><br></div></div></div><div class="gmail_extra"><br><br><div class="gmail_quote">On Mon, Jul 8, 2013 at 5:49 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">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>
<div class="HOEnZb"><div class="h5"><<a href="mailto:carter.schonwald@gmail.com">carter.schonwald@gmail.com</a>> wrote:<br>
> in some of the test examples, if I replace CHECK with CHECK-NEXT, the 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">swlin@post.harvard.edu</a>> 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">carter.schonwald@gmail.com</a>> wrote:<br>
>> > hrmm, I think those loads should not be volatile, because the 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 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">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 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">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 copying<br>
>> >> > that<br>
>> >> > convention. Should I update the patch to change those to check too?<br>
>> >> ><br>
>> >> ><br>
>> >> > On Mon, Jul 8, 2013 at 4:54 PM, Stephen Lin <<a href="mailto:swlin@post.harvard.edu">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 the<br>
>> >> >> order you specify them in the IR; even if they are now, they 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 simply<br>
>> >> >> CHECK? I haven't tried it yet but think that would work and be 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">carter.schonwald@gmail.com</a>> wrote:<br>
>> >> >> > Hey All,<br>
>> >> >> ><br>
>> >> >> > currently the GHC calling convention doesnt have support for using<br>
>> >> >> > the<br>
>> >> >> > AVX<br>
>> >> >> > 256bit width registers. Attached is a patch that augments the 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 the<br>
>> >> >> > SIMD<br>
>> >> >> > support recently added to ghc (as well as the principal author 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 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">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>
</div></div></blockquote></div><br></div>