This is an archive of the discontinued LLVM Phabricator instance.

Fix bug 25440: GVN assertion after coercing loads
ClosedPublic

Authored by weimingz on Nov 6 2015, 11:22 PM.

Diff Detail

Event Timeline

weimingz updated this revision to Diff 39636.Nov 6 2015, 11:22 PM
weimingz retitled this revision from to Fix bug 25440: GVN assertion after coercing loads.
weimingz updated this object.
weimingz added a subscriber: llvm-commits.
dberlin edited edge metadata.Nov 7 2015, 12:28 PM
dberlin added a subscriber: dberlin.

This patch doesn't make sense.

Either you need to have it assign the GV and fix the leader tables so that
the lookup at this point succeeds (which is ideal), or VN.lookup(op) ==
null, it should set success=false and break.

The former is *greatly* preferable to the latter.

What you've done here makes no sense, because findLeader(anything, <new
value number>) will always return nothing, since addLeaderToTable has not
been called on the new value number.
All you are doing in that case is wasting time calling findLeader.

weimingz updated this revision to Diff 39746.Nov 9 2015, 1:35 PM
weimingz edited edge metadata.

This patch is just for review purpose. It only handles the cases in GetLoadValueForLoad().
If the fix is right, I can apply it to other routines like CoerceAvailableValueToLoadType

dberlin added inline comments.Nov 9 2015, 1:47 PM
lib/Transforms/Scalar/GVN.cpp
646

Close, but if you are adding only the new instructions, I believe this should be unconditional.

Think of the leader table as "the list of things in the program that have a given value number".

Even if the value number existed, if this is a new instruction, it's still a new thing that has that value number, and should still end up in the leader table.

(I'd also probably name it addNewInstruction or something)

1209

These definitely look right

weimingz added inline comments.Nov 9 2015, 2:17 PM
lib/Transforms/Scalar/GVN.cpp
646

Is it possible that the new instruction has some existing GVN ? I was thinking calling processInstruction, but concerned that it may have side effect.

dberlin added inline comments.Nov 9 2015, 2:22 PM
lib/Transforms/Scalar/GVN.cpp
646

It is possible the new instruction has an existing VN.

Imagine instruction is the lshr you have below, of a constant and a value.

Imagine we coerce a bunch of loads.

We are highly likely to insert the lshr a bunch of times, and it will be the same each time.
So t's entirely possible that lshr instruction already exists in the program, and vn_lookup_or_add will then find a value number for this instruction.

It happens that the current VN is not great, and so it won't find stuff unless they are pretty much completely identical. processInstruction does a lot of forward propagation to try to make things look the same to account for this.

You also can't call processInstruction here because it may mess with or delete things or change the CFG or ...

(One of the other nice things new gvn does is split analysis and elimination, so that would never happen. It would be hard to fix this in existing GVN)

So i think what you are doing here is pretty much the best you can do in this situation, it should probably just be unconditional, as i mentioned.

weimingz added inline comments.Nov 9 2015, 3:21 PM
lib/Transforms/Scalar/GVN.cpp
646

Cool. Thanks for the detailed explanation.

weimingz updated this revision to Diff 39759.Nov 9 2015, 3:23 PM

This patch set VNs for newly created instructions.

LGTM

It would be super nice to have testcases for all of these, but i suspect that would be pretty hard to test by hand.

I wonder if it doesn't make sense to have, at the end of GVN:

#ifdef DEBUG

for each instruction

assert that VN.lookup returns a non-null value

#endif
to make sure we catch all the cases this can happen in.

Because i suspect it happens in loadpre as well, when phitranslate inserts instructions, etc.

dberlin accepted this revision.Nov 11 2015, 5:29 PM
dberlin edited edge metadata.
This revision is now accepted and ready to land.Nov 11 2015, 5:29 PM
weimingz closed this revision.Nov 12 2015, 10:22 AM