[PATCH] D68408: [InstCombine] Negator - sink sinkable negations

Thu Apr 23 14:40:57 PDT 2020

lebedev.ri added a comment.

In D68408#1998112 <https://reviews.llvm.org/D68408#1998112>, @kcc wrote:

> Hi,

Hi.

> This change causes a performance regression in tsan, as detected on our LLVM buildbot: 
>  http://lab.llvm.org:8011/builders/sanitizer-x86_64-linux-autoconf/builds/49850/steps/tsan%20analyze/logs/stdio

Looks like the build was red already: http://lab.llvm.org:8011/builders/sanitizer-x86_64-linux-autoconf/builds/49808
That explains why i didn't see the new failure: http://lab.llvm.org:8011/builders/sanitizer-x86_64-linux-autoconf/builds/49809

> The script that comes with tsan checks the number of PUSH, etc in some of the key tsan functions, 
>  where each extra PUSH cases tsan to be slower.
> 
> With this change, the number of PUSHes went from 3 to 4.
> 
> Please take a look, this might be a performance regression for a wider set of targets.
> 
> Before your change:
> 
>   read1 tot 484; size 1830; rsp 1; push 3; pop 15; call 2; load 24; store  9; sh  46; mov 106; lea   2; cmp  76
> 
> After your change:
> 
>   read1 tot 515; size 1980; rsp 1; push 4; pop 4; call 2; load 24; store  9; sh  46; mov 113; lea   2; cmp  90

Interesting. Not very unexpected, there's always possibility of an avalanche effect with IR changes.

~~Unhelpful answer: wow how things have regressed since rL342092 <https://reviews.llvm.org/rL342092> / D51985 <https://reviews.llvm.org/D51985>~~

> Script to reproduce (in the llvm-project root dir, with "build" subdir)
> 
> #!/bin/bash
> 
> compile() {
> 
>   clang -c -O2  compiler-rt/lib/tsan/rtl/tsan_rtl.cpp -I compiler-rt/lib -Wall -std=c++14 -Wno-unused-parameter -O2 -g -DNDEBUG    -m64 -fno-lto -fPIC -fno-builtin -fno-exceptions -fomit-frame-pointer -funwind-tables -fno-stack-protector -fno-sanitize=safe-stack -fvisibility=hidden -fno-lto -O3 -gline-tables-only -Wno-gnu -Wno-variadic-macros -Wno-c99-extensions -Wno-non-virtual-dtor -fPIE -fno-rtti -msse3 -Wframe-larger-than=530 -Wglobal-constructors
> 
> }
> 
> git checkout a13dce1d90cba6c55252dee0a2600eab37ffbc44 <https://reviews.llvm.org/rGa13dce1d90cba6c55252dee0a2600eab37ffbc44>
>  (cd build; ninja clang 2> /dev/null)
> compile
>  compiler-rt/lib/tsan/analyze_libtsan.sh tsan_rtl.o  | grep read1
> 
> git checkout 352fef3f11f5ccb2ddc8e16cecb7302a54721e9f <https://reviews.llvm.org/rG352fef3f11f5ccb2ddc8e16cecb7302a54721e9f>
>  (cd build; ninja clang 2> /dev/null)
> compile
>  compiler-rt/lib/tsan/analyze_libtsan.sh tsan_rtl.o  | grep read1

F11782630: old.ll <https://reviews.llvm.org/F11782630> F11782631: old.o <https://reviews.llvm.org/F11782631> F11782633: new.ll <https://reviews.llvm.org/F11782633> F11782636: new.o <https://reviews.llvm.org/F11782636>
llvm-diff report is pretty large, IR instruction-wise, this appears to be a win overall (-639 +609 instructions).
Visually, i think can spot only two IR patterns that we now fail to fold:

  in function _ZN6__tsan10InitializeEPNS_11ThreadStateE:
    in block %if.then88.i:
      >   %22 = xor i64 %xor.i.i.i, -17592186044417
      >   %mul.i.i194.neg.i = add i64 %22, 1
      >   %sub.i = add i64 %mul.i.i194.neg.i, %mul.i.i203.i
      <   %mul.i.i194.i = xor i64 %xor.i.i.i, 17592186044416
      <   %sub.i = sub i64 %mul.i.i203.i, %mul.i.i194.i
    in block %if.then88.1.i:
      >   %35 = xor i64 %xor.i.i.1.i, -17592186044417
      >   %mul.i.i194.neg.1.i = or i64 %mul.i.i203.1.i, 1
      >   %sub.1.i = add i64 %mul.i.i194.neg.1.i, %35
      <   %mul.i.i194.1.i = xor i64 %xor.i.i.1.i, 17592186044416
      <   %sub.1.i = sub i64 %mul.i.i203.1.i, %mul.i.i194.1.i

Filed https://bugs.llvm.org/show_bug.cgi?id=45647

But i believe, i'm supposed to look at the `@__tsan_read1` function, right?
Then the relevant diff is:

  in function __tsan_read1:
    in block %if.then.i1390.i.i.i:
      >   %sub.neg.i1385.i.i.i = sub nsw i64 %and.i19.i1382.i.i.i, %and.i.i1380.i.i.i
      >   %sub.neg.highbits.i1388.i.i.i = lshr i64 %sub.neg.i1385.i.i.i, %and.i.i.i1387.i.i.i
      >   %cmp7.i1389.i.i.i = icmp ne i64 %sub.neg.highbits.i1388.i.i.i, 0
      <   %sub6.i1387.i.i.i = sub nsw i64 0, %sub.i1383.i.i.i
      <   %sub6.highbits.i1388.i.i.i = lshr i64 %sub6.i1387.i.i.i, %and.i.i.i1386.i.i.i
      <   %cmp7.i1389.i.i.i = icmp ne i64 %sub6.highbits.i1388.i.i.i, 0
          %cmp.i1378.i.i.i = icmp ult i64 %xor.i1422.i.i.i, 1125899906842624
      >   %or.cond.i.i.i = or i1 %cmp.i1378.i.i.i, %cmp7.i1389.i.i.i
      >   br i1 %or.cond.i.i.i, label %do.body226.i.i.i, label %if.end86.i.i.i
      <   %or.cond.i.i.i = or i1 %cmp.i1378.i.i.i, %cmp7.i1389.i.i.i
      <   br i1 %or.cond.i.i.i, label %do.body226.i.i.i, label %if.end86.i.i.i

So we've traded `0 - %sub.i1383.i.i.i` for `%and.i19.i1382.i.i.i - %and.i.i1380.i.i.i`
That's it, [un]fortunately, there is nothing else going on..
But thankfully, that explains the problem well.
Pushed rG5a159ed2a8e5a9a6ced73f78e4c64b01d76d3493 <https://reviews.llvm.org/rG5a159ed2a8e5a9a6ced73f78e4c64b01d76d3493>.
Thanks.

Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D68408/new/

https://reviews.llvm.org/D68408