<div dir="ltr"><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex"><span style="font-family:arial,sans-serif;font-size:13px">true for the last part -> changed that to<br>
</span><span style="font-family:arial,sans-serif;font-size:13px">// Also enabled for LE</span><br>// (desired by ARM - smaller code due to more powerful adressing modes)<br><span style="font-family:arial,sans-serif;font-size:13px">to deter other reviewers ;-)</span><br style="font-family:arial,sans-serif;font-size:13px">
<span style="font-family:arial,sans-serif;font-size:13px">For the first part: I still want that here - at the start of the LDR/STR rules, because it is the reason these rules have been added for BE and LE.<br></span><span style="font-family:arial,sans-serif;font-size:13px">It's non-obvious, why NEON register loads are handled along with normal loads.<br>
</span><span style="font-family:arial,sans-serif;font-size:13px">A lot of that paragraph is there by request from James Molloy (other reviewer).<br></span><span style="font-family:arial,sans-serif;font-size:13px">BTW: I just noticed he was dropped from the CCs.</span></blockquote>
<div><span style="font-family:arial,sans-serif;font-size:13px"><br></span></div><div><span style="font-family:arial,sans-serif;font-size:13px">I'm still here :)</span></div><div><span style="font-family:arial,sans-serif;font-size:13px"><br>
</span></div><div><span style="font-family:arial,sans-serif;font-size:13px">I still don't like the "desired by ARM" statement. Enabling both LDR and LD1 for loads is obviously correct, and obviously better than just one (as you have a wider choice of addressing modes). I don't think what ARM wants should come into it, and I wouldn't like this statement to make it into the codebase.</span></div>
<div><span style="font-family:arial,sans-serif;font-size:13px"><br></span></div><div><span style="font-family:arial,sans-serif;font-size:13px">Thanks for doing the other changes I requested!</span></div><div><span style="font-family:arial,sans-serif;font-size:13px"><br>
</span></div><div><span style="font-family:arial,sans-serif;font-size:13px">Cheers,</span></div><div><span style="font-family:arial,sans-serif;font-size:13px"><br></span></div><div><span style="font-family:arial,sans-serif;font-size:13px">James</span></div>
</div><div class="gmail_extra"><br><br><div class="gmail_quote">On 4 March 2014 11:38, Tim Northover <span dir="ltr"><<a href="mailto:t.p.northover@gmail.com" target="_blank">t.p.northover@gmail.com</a>></span> wrote:<br>
<blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">Hi Albrecht,<br>
<div class=""><br>
On 4 March 2014 10:19, Albrecht Kadlec <<a href="mailto:akadlec@a-bix.com">akadlec@a-bix.com</a>> wrote:<br>
> For the first part: I still want that here - at the start of the LDR/STR<br>
> rules, because it is the reason these rules have been added for BE and LE.<br>
> It's non-obvious, why NEON register loads are handled along with normal<br>
> loads.<br>
<br>
</div>Well, fair enough.<br>
<div class=""><br>
> Could we move back to Phabricator for the comments on the "final few" diffs<br>
> ?<br>
<br>
</div>Yep, sounds good to me.<br>
<div class=""><br>
> Sorry to add to your pain: Then we'd logically have to remove the IsBE<br>
> predicate: "Don't add what you can't test". We have no other way of ensuring<br>
> that the code paths stay the same for LE & BE - and that adding a rule for<br>
> LE doesn't kill BE.<br>
<br>
</div>I'm actually much more worried about people introducing unwarranted<br>
similarities between BE and LE than the reverse, which carpet-bombing<br>
the tests with an identical aarch64_be run line is not going to guard<br>
against. In fact it would be more likely cement any unjustified<br>
symmetry.<br>
<br>
Anyone explicitly introducing some dependence on IsBE or IsLE will be<br>
expected to provide tests for that functionality in both directions.<br>
<div class=""><br>
> Unfortunately there's no concept of lower priority weekly tests, that I know<br>
> of. (asan would like that too - Hi Kostya!)<br>
<br>
</div>You could try separate files with "REQUIRES: long_tests". I think one<br>
or two people might actually run them, but not many. Another<br>
reasonably common approach would be to *change* some of the existing<br>
aarch64 tests to aarch64_be in areas that are almost certainly common<br>
(this is how linux & iOS tend to share the 32-bit ARM tests, for<br>
example).<br>
<div class=""><br>
> So I'm fine with removing the identical tests for BE, if you tell me that BE<br>
> is not such an important target that it has to be guarded against<br>
> unintentional changes.<br>
<br>
</div>I think it's not different enough to warrant doubling the test load<br>
for. Similar arguments could be made for PIC vs static, large/small<br>
code models, NEON/no-NEON or any number of other options that affect<br>
how the backend behaves (we're up to "* 16" already).<br>
<br>
But the reality is that they have focussed tests on the areas they're<br>
expected to affect. Testing whether we can add two integers<br>
successfully in PIC mode or make use of "fmla" when the code is<br>
destined for the JIT just isn't productive.<br>
<br>
The blanket approach has proven itself ineffective already, before<br>
even being committed, with:<br>
<div class=""><br>
> neon-copy.ll is one of those that we need to investigate. Why would you say<br>
> that it's wrong - it's just hard-coded to LE ?<br>
<br>
</div>Yes, take the first one for example:<br>
<br>
define <16 x i8> @ins16bw(<16 x i8> %tmp1, i8 %tmp2) {<br>
;CHECK: ins {{v[0-9]+}}.b[15], {{w[0-9]+}}<br>
  %tmp3 = insertelement <16 x i8> %tmp1, i8 %tmp2, i32 15<br>
  ret <16 x i8> %tmp3<br>
}<br>
<br>
On big-endian I believe that should produce "ins v0.b[0], w0" because<br>
that "i32 15" refers to the element of the vector with the highest<br>
address (if it was stored). The rest are the same, as well as most<br>
(but probably not all) other uses of indexes in the tests.<br>
<div class=""><br>
> 1) remove IsLE guards for lane-loads - also for any parts of LD1 ???<br>
<br>
</div>The "normal" ld1s are probably fine as you have them. I wouldn't worry<br>
about being selective there for now, since mostly ldr does the same<br>
job.<br>
<div class=""><br>
> 2) remove AARCH64_BE test RUNs that are equivalent to LE, because BE isn't<br>
> important enough to be tested all the time.<br>
<br>
</div>I'd be reasonably happy for you to change a few from LE to BE (and<br>
just add BE in some important edge cases) *after* careful thought. But<br>
it's a separate issue to this patch, so perhaps it would be best to<br>
discuss it in a separate thread.<br>
<br>
This patch specifically tweaks some load/store functionality, so<br>
that's what it should be testing.<br>
<div class=""><br>
> 3) anything else from another thread I forgot ?<br>
<br>
</div>Not that I can think of.<br>
<br>
Cheers.<br>
<span class="HOEnZb"><font color="#888888"><br>
Tim.<br>
</font></span><div class="HOEnZb"><div class="h5">_______________________________________________<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>
</div></div></blockquote></div><br></div>