nss library miscompilation post r313618

[Quuxplusone]

Bugzilla Link	PR35103
Status	RESOLVED FIXED
Importance	P enhancement
Reported by	Manoj Gupta (manojgupta@google.com)
Reported on	2017-10-26 22:25:27 -0700
Last modified on	2017-12-11 12:57:48 -0800
Version	trunk
Hardware	PC Linux
CC	chh@google.com, eastig@yandex.ru, kristof.beyls@gmail.com, llozano@chromium.org, llvm-bugs@lists.llvm.org, manojgupta@google.com, pirama@google.com, roger.ferreribanez@arm.com, smithp352@googlemail.com, srhines@google.com, yikong@google.com
Fixed by commit(s)
Attachments	nss.tgz (183231 bytes, application/x-compressed-tar) bad.ll (1941 bytes, text/plain)
Blocks
Blocked by
See also

Created attachment 19353
ecp_256 files (good/bad versions)

We are seeing a Unittest fail in ChromeOS when we tried to rebase to a new llvm
version. Bisection pointed to the commit r313618 with  file ecp_256.c in nss
library  miscompiled.

The good and bad versions of the file are attached. The files are compiled at
r313617 (good) and r313618 (bad).

[Quuxplusone]

Attached nss.tgz (183231 bytes, application/x-compressed-tar): ecp_256 files (good/bad versions)

[Quuxplusone]

Around line 106

  /* sum 3 (rest of it)*/
  MP_ADD_CARRY(r6, a14, r6, carry);
  MP_ADD_CARRY(r7, 0, r7, carry);
  r8 += carry;
  carry = 0;

r8 is wrongly computed. Although it is a bit difficult to tell with confidence
that starts failing there due to the random nature of the test.

The fact that r8 happens to be the only signed integer in all these
computations might be a clue.

[Quuxplusone]

To clarify: it might happen that an earlier update to r8 is wrong as well, but it usually happens at that point.

[Quuxplusone]

Hi Manoj I reverted the change in 317092

[Quuxplusone]

Thanks Roger,
I also hope PR34564 that was fixed by r313618 stays as fixed.

[Quuxplusone]

Hi Manoj,

yes I decided to integrate all the fixes for the fallout caused by D35192
itself.

See https://reviews.llvm.org/D35192#868189

Once I fix this codegen issue I plan to integrate it in this change too making
any needed revert easier for everyone.

Kind regards,
Roger

[Quuxplusone]

Reverting r313618 "[ARM] Use ADDCARRY / SUBCARRY" caused regressions in the LNT
benchmarks on Cortex-A53(thumb2) and Coretex-A9(thumb2):

MultiSource/Applications/ALAC/decode/alacconvert-decode - A53(12.12%), A9(9.23%)
MultiSource/Applications/ALAC/encode/alacconvert-encode - A53(5.16%), A9(8.88%)

[Quuxplusone]

Attached bad.ll (1941 bytes, text/plain): Testcase

[Quuxplusone]

I re-landed D35192 with a fix for this.

Thanks for your patience.

[Quuxplusone]

Thanks Roger :)