[PATCH] D20379: Codegen: Fix broken assumption in Tail Merge.

[Kyle Butt via llvm-commits]

I have a fixed version of v8_IT_5.ll
Your short version made it obvious that they're relying on then102 and
then115 becoming the same destination.

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.
https://xkcd.com/1168/

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.

On Wed, Jun 15, 2016 at 12:38 PM, Chad Rosier <mcrosier at codeaurora.org>
wrote:

> mcrosier added a comment.
>
> In http://reviews.llvm.org/D20379#458907, @iteratee wrote:
>
> > 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?
> >
> > In http://reviews.llvm.org/D20379#458728, @mcrosier wrote:
> >
> > > In http://reviews.llvm.org/D20379#457684, @iteratee wrote:
> > >
> > > > 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?
> > >
> > >
> > > 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.
> >
> >
> > In general, we want coverage by correct tests. Incorrect tests are a
> liability, not an asset.
>
>
> 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.
>
> > OK, let's take them one at a time:
>
> > licm-dominance:
>
> >  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
>
>
> 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.
>
> > Thumb2/v8_IT_5.ll:
>
> >  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.
>
>
> 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.
>
> I think the critical checks are:
>
> ; CHECK: it     ne
> ; CHECK-NEXT: cmpne
> ; CHECK-NEXT: bne [[JUMPTARGET:.LBB[0-9]+_[0-9]+]]
>
> 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:
>
>   cmp     r0, #3
>   beq     .LBB0_3
>
>   cmp     r0, #1
>   beq     .LBB0_3
>
>   ...
>
> IMO, we should figure out how to fix the test so it continues to test this
> behavior while passing with your patch.
>
> I've reduced the test a little without changing the CHECKs.  Let me know
> if this works with your patch.
>
>   ; RUN: llc < %s -mtriple=thumbv8 -arm-atomic-cfg-tidy=0 | FileCheck %s
>   ; RUN: llc < %s -mtriple=thumbv7 -arm-atomic-cfg-tidy=0 -arm-restrict-it
> | FileCheck %s
>   ; CHECK: it   ne
>   ; CHECK-NEXT: cmpne
>   ; CHECK-NEXT: bne [[JUMPTARGET:.LBB[0-9]+_[0-9]+]]
>   ; CHECK: cbz
>   ; CHECK-NEXT: %if.else163
>   ; CHECK-NEXT: mov.w
>   ; CHECK-NEXT: b
>   ; CHECK: [[JUMPTARGET]]:{{.*}}%if.else173
>   ; CHECK-NEXT: mov.w
>   ; CHECK-NEXT: bx lr
>   ; CHECK-NEXT: %if.else145
>   ; CHECK-NEXT: mov.w
>
>   %struct.hc = type { i32, i32, i32, i32 }
>
>   define i32 @t(i32 %type) optsize {
>   entry:
>     switch i32 %type, label %if.else173 [
>       i32 3, label %if.then115
>       i32 1, label %if.then102
>     ]
>
>   if.then102:
>     unreachable
>
>   if.then115:
>     br i1 undef, label %if.else163, label %if.else145
>
>   if.else145:
>     %call150 = call fastcc %struct.hc* @foo(%struct.hc* undef, i32
> 34865152) optsize
>     br label %while.body172
>
>   if.else163:
>     %call168 = call fastcc %struct.hc* @foo(%struct.hc* undef, i32
> 34078720) optsize
>     br label %while.body172
>
>   while.body172:
>     br label %while.body172
>
>   if.else173:
>     ret i32 -1
>   }
>
>   declare hidden fastcc %struct.hc* @foo(%struct.hc* nocapture, i32)
> nounwind optsize
>
> Chad
>
> > 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:
>
> >  """
>
> >  That only leaves licm-dominance and Thumb2/v8_IT_5.ll
>
> >
>
> > The optimizer seems to be doing reasonable things in both of those cases.
>
> >
>
> > 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.
>
> >  I could disable the FileCheck lines and open bugs for them.
>
> >  """
>
> >  and a week later on the 25th I said:
>
> >  """
>
> >  Barring comment from anyone else, I'm going to go ahead and remove
>
> >
>
> > LLVM :: CodeGen/Thumb2/v8_IT_5.ll
>
> >  LLVM :: CodeGen/X86/licm-dominance.ll
>
> >  In licm-dominance it's clear that bugpoint has gone too far and the
> optimizer is being perfectly reasonable,
>
> >  and it looks pretty reasonable for v8_IT_5.ll as well.
>
> >  """
>
> >  A comment from you then would have been very helpful.
>
>
>
> http://reviews.llvm.org/D20379
>
>
>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20160615/a36118ed/attachment.html>