<div dir="ltr">I have a fixed version of v8_IT_5.ll<div>Your short version made it obvious that they're relying on then102 and then115 becoming the same destination.</div><div><br></div><div>Maybe you can help me with licm-dominance. I can't seem to write a load in IR that ends up being considered as invariant to save my life.</div><div><a href="https://xkcd.com/1168/">https://xkcd.com/1168/</a><br></div><div><br></div><div>I can tag the loads as invariant, and then the flags are correct, but for the load to be *actually* invariant it also has to be "dereferenceable". I'm not certain what that means or how to write it into the IR. Pointers would be useful.</div></div><div class="gmail_extra"><br><div class="gmail_quote">On Wed, Jun 15, 2016 at 12:38 PM, Chad Rosier <span dir="ltr"><<a href="mailto:mcrosier@codeaurora.org" target="_blank">mcrosier@codeaurora.org</a>></span> wrote:<br><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">mcrosier added a comment.<br>
<span class=""><br>
In <a href="http://reviews.llvm.org/D20379#458907" rel="noreferrer" target="_blank">http://reviews.llvm.org/D20379#458907</a>, @iteratee wrote:<br>
<br>
> OK, the two failing tests are gone, I don't think they were valid to begin with. Are there any other concerns about this change?<br>
><br>
> In <a href="http://reviews.llvm.org/D20379#458728" rel="noreferrer" target="_blank">http://reviews.llvm.org/D20379#458728</a>, @mcrosier wrote:<br>
><br>
> > In <a href="http://reviews.llvm.org/D20379#457684" rel="noreferrer" target="_blank">http://reviews.llvm.org/D20379#457684</a>, @iteratee wrote:<br>
> ><br>
> > > OK, the two failing tests are gone, I don't think they were valid to begin with. Are there any other concerns about this change?<br>
> ><br>
> ><br>
> > In general, we don't want to reduce our test coverage. Please provide additional details as to why you believe these two tests aren't valid and why it's reasonable to delete them.<br>
><br>
><br>
> In general, we want coverage by correct tests. Incorrect tests are a liability, not an asset.<br>
<br>
<br>
</span>I agree. Please understand I just want to make sure we're doing our due diligence here. I'm not trying to impeded your progress.<br>
<span class=""><br>
> OK, let's take them one at a time:<br>
<br>
> licm-dominance:<br>
<br>
> This test has NEVER been correct. I've checked the entire version history. The goal of the test is to verify that LICM checks the dominance relation to make sure a load is guaranteed to be executed. The problem with the test is that with all the undefineds, the compiler is free to make sure the load is guaranteed to be executed. The test also relies on the particular behavior of undefined as setting eax to 0<br>
<br>
<br>
</span>After further investigation I tend to agree that bugpoint was overly aggressive and the reduced test case doesn't appear to be actually testing anything.<br>
<span class=""><br>
> Thumb2/v8_IT_5.ll:<br>
<br>
> There are 4 other v8_it tests, but this one suffers from a similar problem as the one above. It has too many undefineds for the assumptions about the resulting code to ever be correct.<br>
<br>
<br>
</span>I think v8_IT_5.ll could have been better written, but I believe it is testing what it is intended to test. Specifically, that we can predicate a tCMPi8 instruction.<br>
<br>
I think the critical checks are:<br>
<br>
; CHECK: it ne<br>
; CHECK-NEXT: cmpne<br>
; CHECK-NEXT: bne [[JUMPTARGET:.LBB[0-9]+_[0-9]+]]<br>
<br>
If you edit isV8EligibleForIT() in ARMFeatures.h to return false for ARM::tCMPi8 you'll break this test, which is the regression we're trying to avoid. We'll generate code like the following:<br>
<br>
cmp r0, #3<br>
beq .LBB0_3<br>
<br>
cmp r0, #1<br>
beq .LBB0_3<br>
<br>
...<br>
<br>
IMO, we should figure out how to fix the test so it continues to test this behavior while passing with your patch.<br>
<br>
I've reduced the test a little without changing the CHECKs. Let me know if this works with your patch.<br>
<br>
; RUN: llc < %s -mtriple=thumbv8 -arm-atomic-cfg-tidy=0 | FileCheck %s<br>
; RUN: llc < %s -mtriple=thumbv7 -arm-atomic-cfg-tidy=0 -arm-restrict-it | FileCheck %s<br>
; CHECK: it ne<br>
; CHECK-NEXT: cmpne<br>
; CHECK-NEXT: bne [[JUMPTARGET:.LBB[0-9]+_[0-9]+]]<br>
; CHECK: cbz<br>
; CHECK-NEXT: %if.else163<br>
; CHECK-NEXT: mov.w<br>
; CHECK-NEXT: b<br>
; CHECK: [[JUMPTARGET]]:{{.*}}%if.else173<br>
; CHECK-NEXT: mov.w<br>
; CHECK-NEXT: bx lr<br>
; CHECK-NEXT: %if.else145<br>
; CHECK-NEXT: mov.w<br>
<br>
%struct.hc = type { i32, i32, i32, i32 }<br>
<br>
define i32 @t(i32 %type) optsize {<br>
entry:<br>
switch i32 %type, label %if.else173 [<br>
i32 3, label %if.then115<br>
i32 1, label %if.then102<br>
]<br>
<br>
if.then102:<br>
unreachable<br>
<br>
if.then115:<br>
br i1 undef, label %if.else163, label %if.else145<br>
<br>
if.else145:<br>
%call150 = call fastcc %struct.hc* @foo(%struct.hc* undef, i32 34865152) optsize<br>
br label %while.body172<br>
<br>
if.else163:<br>
%call168 = call fastcc %struct.hc* @foo(%struct.hc* undef, i32 34078720) optsize<br>
br label %while.body172<br>
<br>
while.body172:<br>
br label %while.body172<br>
<br>
if.else173:<br>
ret i32 -1<br>
}<br>
<br>
declare hidden fastcc %struct.hc* @foo(%struct.hc* nocapture, i32) nounwind optsize<br>
<span class="HOEnZb"><font color="#888888"><br>
Chad<br>
</font></span><div class="HOEnZb"><div class="h5"><br>
> I feel I've given plenty of time for people to respond to the tests in question. On May 19, almost a month ago I said:<br>
<br>
> """<br>
<br>
> That only leaves licm-dominance and Thumb2/v8_IT_5.ll<br>
<br>
><br>
<br>
> The optimizer seems to be doing reasonable things in both of those cases.<br>
<br>
><br>
<br>
> I don't really want to remove tests from the regression suite, but the tests seem to be relying on bad assumptions, and I'm not the best person to fix them.<br>
<br>
> I could disable the FileCheck lines and open bugs for them.<br>
<br>
> """<br>
<br>
> and a week later on the 25th I said:<br>
<br>
> """<br>
<br>
> Barring comment from anyone else, I'm going to go ahead and remove<br>
<br>
><br>
<br>
> LLVM :: CodeGen/Thumb2/v8_IT_5.ll<br>
<br>
> LLVM :: CodeGen/X86/licm-dominance.ll<br>
<br>
> In licm-dominance it's clear that bugpoint has gone too far and the optimizer is being perfectly reasonable,<br>
<br>
> and it looks pretty reasonable for v8_IT_5.ll as well.<br>
<br>
> """<br>
<br>
> A comment from you then would have been very helpful.<br>
<br>
<br>
<br>
<a href="http://reviews.llvm.org/D20379" rel="noreferrer" target="_blank">http://reviews.llvm.org/D20379</a><br>
<br>
<br>
<br>
</div></div></blockquote></div><br></div>