Writing unit tests - how to test re-orderable blocks...

We have a test that looks like this…

define void @array16_store() {
; CHECK-LABEL: array16_store:

; CHECK: ldi [[REG1:r[0-9]+]], 204
; CHECK: ldi [[REG2:r[0-9]+]], 170
; CHECK: sts int.array+3, [[REG2]]
; CHECK: sts int.array+2, [[REG1]]

; CHECK: ldi [[REG1:r[0-9]+]], 187
; CHECK: ldi [[REG2:r[0-9]+]], 170
; CHECK: sts int.array+1, [[REG2]]
; CHECK: sts int.array, [[REG1]]

; CHECK: ldi [[REG1:r[0-9]+]], 221
; CHECK: ldi [[REG2:r[0-9]+]], 170
; CHECK: sts int.array+5, [[REG2]]
; CHECK: sts int.array+4, [[REG1]]
  store i16 43707, i16* getelementptr inbounds ([3 x i16], [3 x i16]* @int.array, i32 0, i64 0)
  store i16 43724, i16* getelementptr inbounds ([3 x i16], [3 x i16]* @int.array, i32 0, i64 1)
  store i16 43741, i16* getelementptr inbounds ([3 x i16], [3 x i16]* @int.array, i32 0, i64 2)
  ret void
}

The issue I have is this test is unnecessarily fragile because the compiler can (and recently has) decided to reorder the blocks in a perfectly valid way. For example:

; %bb.0:
  ldi r24, 221
  ldi r25, 170
  sts int.array+5, r25
  sts int.array+4, r24
  ldi r24, 204
  ldi r25, 170
  sts int.array+3, r25
  sts int.array+2, r24
  ldi r24, 187
  ldi r25, 170
  sts int.array+1, r25
  sts int.array, r24
  ret

The three blocks should be independent, it doesn’t matter whether it does the store to array+5 and array+4 first or last. What matters is that each block stays together, these instructions must stay together for example:
  ldi r24, 221
  ldi r25, 170
  sts int.array+5, r25
  sts int.array+4, r24

But that could come after the other two blocks or before them.

How can I write FileCheck conditions to test this? If they were single re-orderable lines, I could use CHECK-DAG for each, but I need the four lines of each block to stay together.

Can anyone offer advice?

Carl

We have a test that looks like this…

define void @array16_store() {
; CHECK-LABEL: array16_store:

; CHECK: ldi [[REG1:r[0-9]+]], 204
; CHECK: ldi [[REG2:r[0-9]+]], 170
; CHECK: sts int.array+3, [[REG2]]
; CHECK: sts int.array+2, [[REG1]]

; CHECK: ldi [[REG1:r[0-9]+]], 187
; CHECK: ldi [[REG2:r[0-9]+]], 170
; CHECK: sts int.array+1, [[REG2]]
; CHECK: sts int.array, [[REG1]]

; CHECK: ldi [[REG1:r[0-9]+]], 221
; CHECK: ldi [[REG2:r[0-9]+]], 170
; CHECK: sts int.array+5, [[REG2]]
; CHECK: sts int.array+4, [[REG1]]
store i16 43707, i16* getelementptr inbounds ([3 x i16], [3 x i16]* @int.array, i32 0, i64 0)
store i16 43724, i16* getelementptr inbounds ([3 x i16], [3 x i16]* @int.array, i32 0, i64 1)
store i16 43741, i16* getelementptr inbounds ([3 x i16], [3 x i16]* @int.array, i32 0, i64 2)
ret void
}

The issue I have is this test is unnecessarily fragile because the compiler can (and recently has) decided to reorder the blocks in a perfectly valid way. For example:

; %bb.0:
ldi r24, 221
ldi r25, 170
sts int.array+5, r25
sts int.array+4, r24
ldi r24, 204
ldi r25, 170
sts int.array+3, r25
sts int.array+2, r24
ldi r24, 187
ldi r25, 170
sts int.array+1, r25
sts int.array, r24
ret

The three blocks should be independent, it doesn’t matter whether it does the store to array+5 and array+4 first or last. What matters is that each block stays together, these instructions must stay together for example:
ldi r24, 221
ldi r25, 170
sts int.array+5, r25
sts int.array+4, r24

But that could come after the other two blocks or before them.

How can I write FileCheck conditions to test this? If they were single re-orderable lines, I could use CHECK-DAG for each, but I need the four lines of each block to stay together.

Can anyone offer advice?

Carl

We have a test that looks like this…

define void @array16_store() {
; CHECK-LABEL: array16_store:

; CHECK: ldi [[REG1:r[0-9]+]], 204
; CHECK: ldi [[REG2:r[0-9]+]], 170
; CHECK: sts int.array+3, [[REG2]]
; CHECK: sts int.array+2, [[REG1]]

; CHECK: ldi [[REG1:r[0-9]+]], 187
; CHECK: ldi [[REG2:r[0-9]+]], 170
; CHECK: sts int.array+1, [[REG2]]
; CHECK: sts int.array, [[REG1]]

; CHECK: ldi [[REG1:r[0-9]+]], 221
; CHECK: ldi [[REG2:r[0-9]+]], 170
; CHECK: sts int.array+5, [[REG2]]
; CHECK: sts int.array+4, [[REG1]]

All these CHECK except the first one should be CHECK-NEXT.

  store i16 43707, i16* getelementptr inbounds ([3 x i16], [3 x i16]* @int.array, i32 0, i64 0)
  store i16 43724, i16* getelementptr inbounds ([3 x i16], [3 x i16]* @int.array, i32 0, i64 1)
  store i16 43741, i16* getelementptr inbounds ([3 x i16], [3 x i16]* @int.array, i32 0, i64 2)
  ret void
}

The issue I have is this test is unnecessarily fragile because the compiler can (and recently has) decided to reorder the blocks in a perfectly valid way. For example:
; %bb.0:
        ldi r24, 221
        ldi r25, 170
        sts int.array+5, r25
        sts int.array+4, r24
        ldi r24, 204
        ldi r25, 170
        sts int.array+3, r25
        sts int.array+2, r24
        ldi r24, 187
        ldi r25, 170
        sts int.array+1, r25
        sts int.array, r24
        ret

The three blocks should be independent, it doesn’t matter whether it does the store to array+5 and array+4 first or last. What matters is that each block stays together, these instructions must stay together for example:
        ldi r24, 221
        ldi r25, 170
        sts int.array+5, r25
        sts int.array+4, r24

But that could come after the other two blocks or before them.

How can I write FileCheck conditions to test this? If they were single re-orderable lines, I could use CHECK-DAG for each, but I need the four lines of each block to stay together.

Is the order deterministic, or does it reorder blocks in the same test
differently each time?
If it is *NOT* deterministic, then it is most certainly a bug that
should be reported.
If it is deterministic, (answer without backend-specific knowledge) it
might be best not to worry about this,
and just use the llvm/utils/update_llc_test_checks.py to simplify
updating of the tests.

Can anyone offer advice?

Carl

Roman.

I’m not sure if it’s truly deterministic. It always gives the same results (so far) on my machine but I’m not sure that’s enough.

My guess is it’s probably going to be deterministic on one machine but might well not be deterministic across environments. Like it might give varying results if cross compiled on different hosts, macOS vs intel Linux vs arm vs s390. (Obviously AVR is always a cross compiler as it’s embedded!)

The patch that “broke” the unit test on my timeline is e2baccfd07. This was a commit that improved parallelism on the compile. In this case the blocks are independent. It’s valid to set them in any order.

If there’s no way to get FileCheck to recognise this, the test should probably be weakened or it will keep breaking I guess?

Carl

I’m not sure if it’s truly deterministic. It always gives the same results (so far) on my machine but I’m not sure that’s enough.

My guess is it’s probably going to be deterministic on one machine but might well not be deterministic across environments. Like it might give varying results if cross compiled on different hosts, macOS vs intel Linux vs arm vs s390. (Obviously AVR is always a cross compiler as it’s embedded!)

The patch that “broke” the unit test on my timeline is e2baccfd07. This was a commit that improved parallelism on the compile. In this case the blocks are independent. It’s valid to set them in any order.

So https://github.com/llvm-mirror/llvm/commit/e2baccfd07
If the output is not fully deterministic, it really really sounds like a bug.
Reverse-iteration bot should even catch that in more general case.

If there’s no way to get FileCheck to recognise this, the test should probably be weakened or it will keep breaking I guess?

Which test is that? I'm guessing it's not an upstream one?
Maybe you want https://llvm.org/docs/CommandGuide/FileCheck.html#the-check-dag-directive
?

Carl

Roman.

From: llvm-dev [mailto:llvm-dev-bounces@lists.llvm.org] On Behalf Of Carl
Peto via llvm-dev
Sent: Thursday, March 07, 2019 5:44 PM
To: llvm-dev
Subject: [llvm-dev] Writing unit tests - how to test re-orderable
blocks...

We have a test that looks like this…

define void @array16_store() {
; CHECK-LABEL: array16_store:

; CHECK: ldi [[REG1:r[0-9]+]], 204
; CHECK: ldi [[REG2:r[0-9]+]], 170
; CHECK: sts int.array+3, [[REG2]]
; CHECK: sts int.array+2, [[REG1]]

; CHECK: ldi [[REG1:r[0-9]+]], 187
; CHECK: ldi [[REG2:r[0-9]+]], 170
; CHECK: sts int.array+1, [[REG2]]
; CHECK: sts int.array, [[REG1]]

; CHECK: ldi [[REG1:r[0-9]+]], 221
; CHECK: ldi [[REG2:r[0-9]+]], 170
; CHECK: sts int.array+5, [[REG2]]
; CHECK: sts int.array+4, [[REG1]]
  store i16 43707, i16* getelementptr inbounds ([3 x i16], [3 x i16]*
@int.array, i32 0, i64 0)
  store i16 43724, i16* getelementptr inbounds ([3 x i16], [3 x i16]*
@int.array, i32 0, i64 1)
  store i16 43741, i16* getelementptr inbounds ([3 x i16], [3 x i16]*
@int.array, i32 0, i64 2)
  ret void
}

The issue I have is this test is unnecessarily fragile because the
compiler can (and recently has) decided to reorder the blocks in a
perfectly valid way. For example:

; %bb.0:
  ldi r24, 221
  ldi r25, 170
  sts int.array+5, r25
  sts int.array+4, r24
  ldi r24, 204
  ldi r25, 170
  sts int.array+3, r25
  sts int.array+2, r24
  ldi r24, 187
  ldi r25, 170
  sts int.array+1, r25
  sts int.array, r24
  ret

The three blocks should be independent, it doesn’t matter whether it does
the store to array+5 and array+4 first or last. What matters is that each
block stays together, these instructions must stay together for example:
  ldi r24, 221
  ldi r25, 170
  sts int.array+5, r25
  sts int.array+4, r24

But that could come after the other two blocks or before them.

How can I write FileCheck conditions to test this? If they were single re-
orderable lines, I could use CHECK-DAG for each, but I need the four lines
of each block to stay together.

I would capture the text output to a file, and run FileCheck multiple times
on the same file, once for each group of instructions. You would need to give
each block of CHECK lines its own prefix, but that's a small thing.

But I would second the observation that even if different orders are valid,
the compiler's behavior should be deterministic, aside from patches that
change the ordering for a specific reason. This kind of breakage ought to
be unusual, and if it's not, that's likely a bug that should be fixed
instead of you working around it in your test.
--paulr

I’ve corrected the test to match current output in my patch. I think from what you guys say that’s perfectly good enough.

I’ll submit to the AVR team via the maintainer.

Thanks for your help guys. :slight_smile:

Carl

I’m not sure if it’s truly deterministic. It always gives the same results (so far) on my machine but I’m not sure that’s enough.

My guess is it’s probably going to be deterministic on one machine but might well not be deterministic across environments. Like it might give varying results if cross compiled on different hosts, macOS vs intel Linux vs arm vs s390. (Obviously AVR is always a cross compiler as it’s embedded!)

The patch that “broke” the unit test on my timeline is e2baccfd07.

A subversion revision number would be helpful to identify this since we’re still mostly using them (eg: one can’t search the commit mailing list for a git hash).

This was a commit that improved parallelism on the compile. In this case the blocks are independent. It’s valid to set them in any order.

It’s generally acceptable that certain kinds of changes perturb certain tests - it’d be good to know why, though. And to be sure that the program behavior is deterministic (same result for the same input no matter what machine it’s run on).

But making tests resilient to unrelated changes is important too. If it’s worth it in this case (maybe check related tests for similar things, see if they avoid being this brittle, etc) then multiple FileCheck invocations would be one way to go (each one checking some portion of the output) as others have mentioned.