This is an archive of the discontinued LLVM Phabricator instance.

Fix flakiness in fp16-promote.ll
ClosedPublic

Authored by pirama on Apr 20 2015, 10:28 AM.

Details

Summary

In the f16-promote test, make the checks for native conversion instructions
similar to the libcall checks:

  • Remove hard coded register names
  • Do not check exact instruction sequences.

This fixes test flakiness due to non-determinism in instruction
scheduling and register allocation. I also fixed a few minor things in
the CHECK-LIBCALL checks.

I'll try to find a way to check that unnecessary loads, stores, or
conversions don't happen.

Diff Detail

Repository
rL LLVM

Event Timeline

pirama updated this revision to Diff 24028.Apr 20 2015, 10:28 AM
pirama retitled this revision from to Fix flakiness in fp16-promote.ll.
pirama updated this object.
pirama edited the test plan for this revision. (Show Details)
pirama added reviewers: mzolotukhin, srhines, ab.
pirama added a subscriber: Unknown Object (MLST).
pirama updated this revision to Diff 24029.Apr 20 2015, 10:35 AM

Patch 2. Fix accidental change of the mattr flag from vfp3 to vfp4.

ab edited edge metadata.Apr 20 2015, 10:42 AM

I'm fine with this, even though I'd prefer the very explicit tests, for instance with [[R1:r[0-9]+]] patterns to not care about register allocation. It seems to me that we're at a point in the project's life where it's much more important to avoid regressions than to make it easy to add new features.

Note that one advantage of just checking for instructions is that, for the -LIBCALL/-FP16, one trick you can use to avoid duplicating the entire checks is to have an initial pattern, but then test everything using that pattern in generic CHECK: lines. So, at the very first match, you'd have:

; CHECK-LABEL: test_fadd:
; CHECK-FP16: [[F16_TO_F32:vcvtb.f32.f16]]
; CHECK-LIBCALL: [[F16_TO_F32:bl __gnu_h2f_ieee]]
; CHECK: [[F32_TO_F16]]
; CHECK: vadd.f32
; CHECK-FP16: [[F32_TO_F16:vcvtb.f16.f32]]
; CHECK-LIBCALL: [[F32_TO_F16:bl __gnu_f2h_ieee]]

That lets you avoid duplicating most of the remaining CHECK lines:

; CHECK-LABEL: test_fsub:
; CHECK: [[F16_TO_F32]]
; CHECK: [[F16_TO_F32]]
; CHECK: vsub.f32
; CHECK: [[F32_TO_F16]]

(with the appropriate --check-prefix=CHECK, or CHECK-ALL like you have)

test/CodeGen/ARM/fp16-promote.ll
886 ↗(On Diff #24028)

Without -NEXT, there's no point in checking .fnstart, right?

I can probably add regular expressions to match registers. It is still a problem that a different instruction schedule will cause the test to fail.

Is it possible that an explicit target CPU or another option in the command line will remove the variations? If so, we should do that instead of changing the test. I am not familiar enough with the backend to know this.

ab added a subscriber: rengolin.Apr 20 2015, 3:57 PM

I can probably add regular expressions to match registers. It is still a problem that a different instruction schedule will cause the test to fail.

Is it possible that an explicit target CPU or another option in the command line will remove the variations? If so, we should do that instead of changing the test. I am not familiar enough with the backend to know this.

If you set an explicit triple, IIRC there's a default CPU chosen (in this case probably cortex-a8), so I don't expect that to vary. Schedule variations are still possible because of other changes missing from trunk though, probably like the register allocation issue here. (+ Renato, who would have the advice you're looking for)

In any case, bots are still broken, so this should be fixed one way or the other, soon ;)

-Ahmed

In any case, bots are still broken, so this should be fixed one way or the other, soon ;)

How about I commit this patch (sans the .fnstart), so I don't block others? I'll either add explicit register checks or merge the duplicate checks in a followup patch after we decide about the right strategy?

ab added a comment.Apr 20 2015, 4:35 PM

That sounds like the pragmatic solution, and less noisy than reverting, so a reluctant LGTM ;)
I'll let Michael accept, if it fixes the issue.

Thanks!
-Ahmed

mzolotukhin accepted this revision.Apr 20 2015, 4:56 PM
mzolotukhin edited edge metadata.

Yep, please commit the patch to fix the bots, and then follow up as needed later.

Thanks!

This revision is now accepted and ready to land.Apr 20 2015, 4:56 PM
This revision was automatically updated to reflect the committed changes.
pirama added a comment.EditedApr 20 2015, 5:01 PM

Thanks Ahmad, Michael.

For future reference, how do I check the status of the bots? In lab.llvm.org:8011/grid, I see the nightly tests are failing, but couldn't find any reference to a failure of this test.