Bug 69320 - [6 Regression] wrong code generation at -O2 and higher
Summary: [6 Regression] wrong code generation at -O2 and higher
Status: RESOLVED FIXED
Alias: None
Product: gcc
Classification: Unclassified
Component: tree-optimization (show other bugs)
Version: 6.0
: P1 normal
Target Milestone: 6.0
Assignee: Jeffrey A. Law
URL:
Keywords: wrong-code
: 69322 69325 69326 69342 (view as bug list)
Depends on:
Blocks: 69325 69326
  Show dependency treegraph
 
Reported: 2016-01-16 22:36 UTC by James Almer
Modified: 2016-01-19 06:51 UTC (History)
3 users (show)

See Also:
Host:
Target:
Build:
Known to work: 5.3.0
Known to fail: 6.0
Last reconfirmed: 2016-01-17 00:00:00


Attachments
Assembly and preprocessed output of miscompiled file (249.75 KB, application/x-gzip)
2016-01-16 22:36 UTC, James Almer
Details

Note You need to log in before you can comment on or make changes to this bug.
Description James Almer 2016-01-16 22:36:13 UTC
Created attachment 37377 [details]
Assembly and preprocessed output of miscompiled file

http://fate.ffmpeg.org/report.cgi?time=20160116195307&slot=x86_64-archlinux-gcc-experimental
https://git.videolan.org/?p=ffmpeg.git;a=blob;f=libavcodec/svq1enc.c;h=1e1745e7b10ed24e8a600d643b5aa951317f2aba;hb=HEAD

FFmpeg's svq1 encoder is being miscompiled by GCC 6 starting with revision r232361. I'm attaching example assembly of the last good commit and the first bad, alongside the preprocessed output.

To reproduce the issue you can download ffmpeg and run the fate svq1 tests (no external samples needed to run), which will fail and create output video files full of artifacts from the revision mentioned above onward:
git clone https://git.videolan.org/git/ffmpeg.git && cd ffmpeg && ./configure && make fate-vsynth{1,2,3}-svq1
Comment 1 Jeffrey A. Law 2016-01-17 07:27:31 UTC
THanks for the .i and .s files.  The one thing that is missing that would help a lot is the compiler flags used.  Without that information we have to guess and with the number of flags, it's easy to guess wrong.

Thankfully something seems to have recorded the flags and embedded them into your assembly output files.  So was able to use those.  Please if you file future reports make sure to include the compiler flags too.

Anyway, it's a mis-compilation of svq1`_encode_plane.  It's quite strange though.

So we have:


  _373 = _370 <= _372;
  best_374 = (int) _373;

That establishes best_374 as having a boolean range, even though it's an integer variable.

That's important because later we see a conditional on best_374 and infer its value on both arms of the conditional.

So if we look further we have:

;;   basic block 444, loop depth 2, count 0, freq 1, maybe hot
;;    prev block 443, next block 445, flags: (NEW, REACHABLE)
;;    pred:       441 [71.0%]  (FALSE_VALUE,EXECUTABLE)
;;                440 [50.0%]  (FALSE_VALUE,EXECUTABLE)
  if (best_374 == 1)
    goto <bb 445>;
  else
    goto <bb 452>;

If we follow the TRUE arm where we know best_374 will have the value 1 we have this path (there's others, this is just one):

445->445->446->461->451->462->453

Where BB453 looks like:

  # best_3287 = PHI <0(443), best_374(462), 0(429)>


After my patch that statement looks like:

  # best_3287 = PHI <0(443), 0(461), 0(429)>


Yow!  That's clearly not right.    

Still digging, but that looks like a real problem, and hopefully it's the mis-compilation we're looking for.  Certainly if I block the propagation of best_374 via a dbg-counter the resulting code passes its internal tests.
Comment 2 Jeffrey A. Law 2016-01-17 07:51:21 UTC
Ha!  Got it.

On the path noted in the earlier comment we have the following:

<bb 444>:
pred_x ={v} {CLOBBER};
pred_y ={v} {CLOBBER};
_423 = s_46(D)->rd_total;
_424 = score[best_374];
_425 = (long int) _424;
_426 = _423 + _425;
s_46(D)->rd_total = _426;
if (best_374 != 2)
  goto <bb 445>;
else
  goto <bb 449>;



I'd looked at that block a few times and even traced where that test came from, the propagation of best_374 into that test and determined that while suboptimal, it was correct (the test originally used a different variable, one which had the range [0..2].  The path by which the "2" appears in that range eventually gets proved unexecutable and removed.  Once that happens the original object becomes equivalent to best_374 and the copy propagation occurs in copyprop (which AFAICT doesn't try to use range information to simplify things).

Anyway, all that is irrelevant to the problem.  ssa_name_has_boolean_range does something like this

edge_info->rhs = (integer_zerop (op1) ? true_val : false_val);

When it only had to deal with booleans, that was fine because op1 in this context would always be the constant 0 or 1 (it's the RHS of the conditional).

But once we start handling integers with a narrow range, that test is wrong.

Fixing is trivial.  It'll probably take longer to boil down a suitable testcase -- it's late.  I'll poke at this further over the weekend.
Comment 3 Jakub Jelinek 2016-01-17 21:36:46 UTC
Perhaps look at PR69325 or PR69326?
Comment 4 Jakub Jelinek 2016-01-18 08:26:20 UTC
*** Bug 69325 has been marked as a duplicate of this bug. ***
Comment 5 Jakub Jelinek 2016-01-18 08:26:39 UTC
*** Bug 69326 has been marked as a duplicate of this bug. ***
Comment 6 Jakub Jelinek 2016-01-18 08:26:58 UTC
*** Bug 69342 has been marked as a duplicate of this bug. ***
Comment 7 Markus Trippelsdorf 2016-01-18 08:34:05 UTC
*** Bug 69322 has been marked as a duplicate of this bug. ***
Comment 8 Jeffrey A. Law 2016-01-18 19:00:49 UTC
Jakub.  I'll certainly backtest against the referenced BZs

While the fix is trivial, I'm seriously considering just turning the exploitation of ranges off for gcc-6.  It didn't help as much as I had wanted in the path splitting bits, so it's not terribly important for this release.
Comment 9 Jeffrey A. Law 2016-01-18 19:36:17 UTC
I've backtested against all 4 duplicates and my change fixes all of them.
Comment 10 Jeffrey A. Law 2016-01-19 06:44:26 UTC
Author: law
Date: Tue Jan 19 06:43:54 2016
New Revision: 232548

URL: https://gcc.gnu.org/viewcvs?rev=232548&root=gcc&view=rev
Log:
2016-01-18  Jeff Law  <law@redhat.com>

	PR tree-optimization/69320
	* tree-ssa-dom.c (record_edge_info): For comparisons against a boolean
	ranged object, do nothing if the RHS constant is not [0..1].
	(optimize_stmt): Comparing a boolean ranged object against a
	constant outside [0..1] results in a compile-time constant.

	* tree-ssanames.c (ssa_name_has_boolean_range): Remove unnecessary
	test.

	PR tree-optimization/69320
	* gcc.c-torture/pr69320-1.c: New test.
	* gcc.c-torture/pr69320-2.c: New test.
	* gcc.c-torture/pr69320-3.c: New test.
	* gcc.c-torture/pr69320-4.c: New test.

Added:
    trunk/gcc/testsuite/gcc.c-torture/execute/pr69320-1.c
    trunk/gcc/testsuite/gcc.c-torture/execute/pr69320-2.c
    trunk/gcc/testsuite/gcc.c-torture/execute/pr69320-3.c
    trunk/gcc/testsuite/gcc.c-torture/execute/pr69320-4.c
Modified:
    trunk/gcc/ChangeLog
    trunk/gcc/testsuite/ChangeLog
    trunk/gcc/tree-ssa-dom.c
    trunk/gcc/tree-ssanames.c
Comment 11 Jeffrey A. Law 2016-01-19 06:50:53 UTC
Fixed by commit on the trunk.
Comment 12 Jeffrey A. Law 2016-01-19 06:51:27 UTC
Fixed on trunk.