[PATCH] D63629: [JumpThreading][PR42085] Fold constant TIs before calculating LoopHeaders

[Brian Rzycki via Phabricator via llvm-commits]

brzycki added a comment.

Hello @efriedma,

> How do we actually get a miscompile?

The new test-case below, `pr42085-loopheader-detection.ll` exhibits the miscompile on unpatched tip LLVM. In PR42085 <https://bugs.llvm.org/show_bug.cgi?id=42085> I've attached a test harness which runs the code using a simple `main.c` and a shell script. The `main.c` looks like:

  #include <libgen.h>
  #include <stdio.h>
  #include <stdlib.h>
  
  extern int test(unsigned char);
  
  char *name;
  
  int main(int arc, char *argv[]) {
    unsigned char arg = atoi(argv[1]);
    name = basename(argv[0]);
    printf("    [%-10s] test == %d\n", name, test(arg));
    return 0;
  }

And we compile `pr42085-loopheader-detection.ll` two different ways to thread and not thread the code:

  echo build main.o
  "$clang" -O3 -c "$main" $debug -o main.o
  
  echo build pr.o
  "$llc" -O0 "$pr" -o pr.s
  "$clang" -c pr.s -o pr.o
  
  echo build jt.o
  "$opt" -passes=jump-threading -S -o jt.ll "$pr"
  "$llc" -O0 jt.ll -o jt.s
  "$clang" -c jt.s -o jt.o
  
  echo link
  "$clang" jt.o main.o -o runme_jt
  "$clang" pr.o main.o -o runme_pr

In the above code `pr.o` is not JumpThreaded while `jt.o` is.

the output of execution shows the miscompile. When input of `(0, 0)` or `(0, 1)` is passed into `test()` we see different returned values:

  [ INPUT ==> arg1=0 arg1=0 ]
      [runme_pr  ] test == 2
      [runme_jt  ] test == 3
  
  [ INPUT ==> arg1=0 arg1=1 ]
      [runme_pr  ] test == 2
      [runme_jt  ] test == 3
  
  [ INPUT ==> arg1=1 arg1=0 ]
      [runme_pr  ] test == -1
      [runme_jt  ] test == -1
  
  [ INPUT ==> arg1=1 arg1=1 ]
      [runme_pr  ] test == -1
      [runme_jt  ] test == -1

In the miscompile case we thread `body1 ==> body2 ==> latch1` to `body1 ==> body2.thread ==> latch1`. We find the following backedges from `FindFunctionBackedges()`. We store the destination `Edge.second` as each `BB` entry in `LoopHeaders`:

  latch2 ==> body1
  body1  ==> pre_head

With the fix (removing trivial constant terminators) before calculating `LoopHeaders`, the function `FindFunctionBackedges()` finds the following edges:

  body1 ==> body2
  body1 ==> pre_head

And correctly promoting `body2` to a loop header pessimizes threads across it.

> I'm worried this fix is covering up a bug in our handling of [loop header] code.

It's entirely possible, I'm not an expert on loop transforms. The reason I went down this path is I noticed the change in `LoopHeaders` when I made the following single change to the IR:

  --- pr42085.ll  2019-06-27 09:55:57.501853639 -0500
  +++ pr42085_good.ll     2019-06-27 10:36:57.667534318 -0500
  @@ -15,7 +15,7 @@
     br i1 %ARG1, label %exit, label %head2
  
   head2:
  -  br i1 false, label %head3, label %body2
  +  br label %body2
  
   head3:
     %tmp8 = add i32 %tmp3, 1

Using the above IR with  `opt -S -jump-threading` on an unpatched LLVM compiler does not exhibit the miscompile. But, as you say, this may be masking a deeper issue that I haven't noticed. I'd definitely appreciate your expertise or any suggestions you may have.


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

https://reviews.llvm.org/D63629