Page MenuHomePhabricator

[Reassociation] Place moved instructions after landing pads
ClosedPublic

Authored by greened on Apr 24 2019, 1:37 PM.

Details

Summary

Reassociation's NegateValue moved instructions to the beginning of
blocks (after PHIs) without checking for landingpad instructions.
It's possible for reassociation to happen within a landingpad block so
we need to make sure we don't move things too early in the block.
This change advances the insertion point past any landingpad.

If the insertion point is then a catchswitch, don't try to reuse an
existing neg but just create a new one at BI, as if we hadn't found
an existing neg at all.

Diff Detail

Repository
rL LLVM

Event Timeline

greened created this revision.Apr 24 2019, 1:37 PM
greened updated this revision to Diff 197160.Apr 29 2019, 12:19 PM

Rebased on latest master and fixed test name.

efriedma added inline comments.
llvm/lib/Transforms/Scalar/Reassociate.cpp
874 ↗(On Diff #197160)

Explicitly checking for LandingPad isn't sufficient on Windows. There are two issues: one, you need to use isEHPad() to check for all the relevant exception-handling instructions, and two, you can't insert a negate into a block with a catchswitch.

greened marked an inline comment as done.Apr 30 2019, 12:52 PM
greened added inline comments.
llvm/lib/Transforms/Scalar/Reassociate.cpp
874 ↗(On Diff #197160)

This code moves the negate around within the local block, so presumably the negate is already in the block and therefore there should be no catchswitch, right?

I will update this to use isEHPad. Thanks!

efriedma added inline comments.Apr 30 2019, 1:04 PM
llvm/lib/Transforms/Scalar/Reassociate.cpp
874 ↗(On Diff #197160)

This code moves the negate around within the local block

No? It moves the negate into the same block as the definition of "V", which as far as I can tell is not the same block where the "sub" instruction was originally located. (There's a dominance relationship, but that isn't really helpful here.)

In particular, the case I'm worried about is where "V" is a phi in the same basic block as a catchswitch.

greened updated this revision to Diff 197422.Apr 30 2019, 1:06 PM

Updated to use isEHPad.

greened marked an inline comment as done.Apr 30 2019, 1:07 PM
greened added inline comments.
llvm/lib/Transforms/Scalar/Reassociate.cpp
874 ↗(On Diff #197160)

Ack, you are right. In the code where I observed the crash they were in the same block but it does not have to be so. Will update.

greened marked an inline comment as done.Apr 30 2019, 1:37 PM
greened added inline comments.
llvm/lib/Transforms/Scalar/Reassociate.cpp
874 ↗(On Diff #197160)

Hrm. If we get into the situation where V is a PHI and there's a catchswitch right after it, what should we do? Just bail out of the loop and create a new neg before BI? That seems better than trying to replicate the neg at each destination of the catchswitch. I am not at all familiar with this code so I don't know what the implications are.

greened updated this revision to Diff 197429.Apr 30 2019, 1:38 PM
greened edited the summary of this revision. (Show Details)

Bail out of the loop that found an existing neg if there is a catchswitch
and just create a new neg instead.

greened edited the summary of this revision. (Show Details)Apr 30 2019, 1:39 PM
greened marked 2 inline comments as done.Apr 30 2019, 1:41 PM
efriedma added inline comments.Apr 30 2019, 2:33 PM
llvm/lib/Transforms/Scalar/Reassociate.cpp
883 ↗(On Diff #197429)

I don't think this does what you want; isEHPad() is true for catchswitch. (It would be nice to have a testcase.)

I'm fine with bailing out of the loop in this rare case.

greened marked an inline comment as done.May 1 2019, 10:36 AM
greened added inline comments.
llvm/lib/Transforms/Scalar/Reassociate.cpp
883 ↗(On Diff #197429)

You are right. Unfortunately, I am not confident I could construct a valid artificial testcase using catchswitch.

greened updated this revision to Diff 197590.May 1 2019, 10:39 AM

Updated to account for isEHPad including catchswitch. I'm not very happy with the hacky use of FoundCatchSwitch but could not think of a way to do this that keeps things relatively clear/readable and doesn't put the for loop into a deeper nesting level or completely reformat the function.

greened marked an inline comment as done.May 1 2019, 10:39 AM

To produce a catchswitch with a PHI, you can run "clang --target=aarch64-pc-windows-msvc -S -emit-llvm -O2" over something like the following:

void g();
void g2();
void use(int, int);
void f(bool b) {
  int z;
  try {
    if (b) {
      z = 3;
      g();
    } else {
      z = 5;
      g2();
    }
  } catch (...) {
    return use(z,-z);
  }
}

You should be able to modify that to produce an appropriate testcase.

greened updated this revision to Diff 198456.May 7 2019, 6:43 AM

Added a test for catchswitch and fixed a bug with falling off the end of a basic block.

efriedma accepted this revision.May 7 2019, 2:58 PM

LGTM with a minor comment.

llvm/lib/Transforms/Scalar/Reassociate.cpp
877 ↗(On Diff #198456)

Maybe adjust this comment a little? V could also be a PHI in a non-landingpad block.

This revision is now accepted and ready to land.May 7 2019, 2:58 PM
This revision was automatically updated to reflect the committed changes.

Looks like some lines got truncated from the head of the first test file. Test is failing due to no run line.

Looks like some lines got truncated from the head of the first test file. Test is failing due to no run line.

I think Reid checked in a fix before I could get to it. Thanks for the report.