[PATCH] D78157: [AArch64InstrInfo] Ignore debug insts in areCFlagsAccessedBetweenInstrs [6/10]

Fri Apr 24 10:16:06 PDT 2020

vsk added a comment.

In D78157#2001436 <https://reviews.llvm.org/D78157#2001436>, @mstorsjo wrote:

> This caused a misoptimization for me. With https://martin.st/temp/scene_sad-preproc.c as reproducible source for the issue, I have the following case:
>
>   $ clang-good -target aarch64-w64-mingw32 scene_sad-preproc.c -O3 -S -o good
>   $ clang-bad -target aarch64-w64-mingw32 scene_sad-preproc.c -O3 -S -o bad
>   $ diff -u good bad
>   --- good        2020-04-24 11:46:41.372102971 +0300
>   +++ bad 2020-04-24 11:46:28.440377143 +0300
>   @@ -156,17 +156,16 @@
>           ldrb    w14, [x14, #1]
>           add     x12, x12, #2            // =2
>           subs    w15, w15, w16
>   -       sub     w13, w13, w14
>   +       subs    w13, w13, w14
>           cneg    w14, w15, mi
>   -       cmp     w13, #0                 // =0
>           cneg    w13, w13, mi
>
>
> (With two other unrelated benign differences removed from the diff for clarity.)
>
> In this case, the comparison can't be hoisted into `subs` past the `cneg`, as that does use the condition flags.

@mstorsjo thanks for reporting this, and sorry about the breakage. The problem is that explicit conversion in MachineInstrBundleIterator for reverse iterators increments the reverse iterator (`MachineInstrBundleIterator(++I.getReverse())`), and I missed this fact. Actually the comment there explains "resulting iterator will dereference ... to the previous node, which is somewhat unexpected; but converting the two endpoints in a range will give the same range in reverse". This sort of thing seems easy to mess up, and it means 'reversedInstructionsWithoutDebug' will not behave as advertised. I plan to get rid of that helper and check in a regression test.

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D78157