[PATCH] D59936: SimplifyCFG SinkCommonCodeFromPredecessors: Also sink function calls without used results (PR41259)

Fri Apr 12 05:16:19 PDT 2019

hans added a comment.

In D59936#1454565 <https://reviews.llvm.org/D59936#1454565>, @hans wrote:

> In D59936#1454346 <https://reviews.llvm.org/D59936#1454346>, @dlj wrote:
>
> > This revision causes tests to fail under ASAN. After some investigation (with Chandler's help), it looks like the safest course of action is to revert. We're following up with Hans separately.
> >
> > Reverted as r357667.
>
>
> Thanks, and sorry for the breakage.
>
> For my own notes, the internal bug tracker entry is 129905821. The V8 team also saw asan errors from ICU due to this, tracked in https://bugs.chromium.org/p/v8/issues/detail?id=9086 I'll try to investigate there to keep this in the open.

And https://bugs.llvm.org/show_bug.cgi?id=41481 for the asan issue.

This points to an interesting point that was raised here earlier though:

In D59936#1452855 <https://reviews.llvm.org/D59936#1452855>, @dmgreen wrote:

> For lifetime.end, if we start producing llvm.lifetime.end(phi(x, y, z)) then I could see that potentially being worse. I have no evidence of that happening though. And it may even be better if it allows further sinking.

We now know that it did happen :-) In the case with the asan false positive it was two lifetime.start intrinsics sunk into one with a select argument.

But I think that is working as intended. It doesn't break any invariants with the intrinsics that I know of, and I don't think it will generate worse code either. In fact, I think seeing those lifetime intrinsics getting sunk shows that the intrinsics were blocking sinking optimizations that we are now able to perform.

Repository:
  rL LLVM

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

https://reviews.llvm.org/D59936