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

Chad Rosier via llvm-commits llvm-commits at lists.llvm.org
Wed Jun 15 12:38:36 PDT 2016

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: [[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 {
    switch i32 %type, label %if.else173 [
      i32 3, label %if.then115
      i32 1, label %if.then102
    br i1 undef, label %if.else163, label %if.else145
    %call150 = call fastcc %struct.hc* @foo(%struct.hc* undef, i32 34865152) optsize
    br label %while.body172
    %call168 = call fastcc %struct.hc* @foo(%struct.hc* undef, i32 34078720) optsize
    br label %while.body172
    br label %while.body172
    ret i32 -1
  declare hidden fastcc %struct.hc* @foo(%struct.hc* nocapture, i32) nounwind optsize


> 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.


More information about the llvm-commits mailing list