Page MenuHomePhabricator

Fix for CFI type tests lowering assert.
ClosedPublic

Authored by dmikulin on Nov 9 2017, 12:45 PM.

Details

Summary

My attempt to fix https://bugs.llvm.org/show_bug.cgi?id=35252.

The problem is that current implementation of Value::replaceUsesExceptBlockAddr() uses UseList iterator to walk the list which keeps changing inside the loop. doRAUW() gets around it by checking if the list is empty to terminate. When we skip blockaddress uses, the list is never empty if there are any blockaddress uses. This change removes blockaddress uses from the UseList, places them in a temporary list and adds them back after all uses are processed.

I don't particularly like this fix, but couldn't find a better way. I have another version where the loop terminating condition checks if the only remaining uses are blockaddresses and skips leading blockaddresses on loop entry, but it requires repeated scans of UseList, so I liked it even less. If anyone has suggestions I'll be happy to explore.

Diff Detail

Repository
rL LLVM

Event Timeline

dmikulin created this revision.Nov 9 2017, 12:45 PM
filcab edited edge metadata.Nov 10 2017, 10:28 AM

Seems ok assuming that construct is safe.

llvm/lib/IR/Value.cpp
469 ↗(On Diff #122309)

Does this pattern exist somewhere else in llvm?

dmikulin added inline comments.Nov 10 2017, 12:52 PM
llvm/lib/IR/Value.cpp
469 ↗(On Diff #122309)

You mean U.set(nullptr);? It's used in a few places as a way to remove a value from UseList without replacing it with anything else. There's no direct access to UseList add/remove methods. As as side effect of set(nullptr), the value ptr gets nuked, so I need to save it to restore later.

Nag... Can you guys please review this?

pcc added inline comments.Nov 16 2017, 11:15 AM
llvm/lib/IR/Value.cpp
477 ↗(On Diff #122309)

I think the only way that you can end up replacing multiple users during an iteration of this loop is if this function gets called. So could you instead collect a set of constants that you need to call handleOperandChange on and take care of them separately after the loop?

dmikulin updated this revision to Diff 123234.Nov 16 2017, 12:34 PM

Changed the fix to save unique Constant users and process them separately outside of the UseList iterator.

davide added inline comments.Nov 16 2017, 12:38 PM
llvm/lib/IR/Value.cpp
459 ↗(On Diff #123234)

SmallSetVector to make sure this is deterministic, maybe?

davide edited edge metadata.Nov 16 2017, 12:38 PM

(unless you don't care about the order at all, in which case you might consider adding a comment).

dmikulin marked an inline comment as done.Nov 16 2017, 1:02 PM
dmikulin added inline comments.
llvm/lib/IR/Value.cpp
459 ↗(On Diff #123234)

Not sure about the order, so changed it to SmallSetVector.

pcc accepted this revision.Nov 16 2017, 2:02 PM

Assuming that you've made the obvious change to the set type locally, this LGTM.

llvm/test/Transforms/LowerTypeTests/blockaddress-2.ll
11 ↗(On Diff #123234)

If this line isn't necessary to reproduce the bug, please remove it.

16 ↗(On Diff #123234)

You can probably remove the argument list and return void.

20 ↗(On Diff #123234)

Same here.

This revision is now accepted and ready to land.Nov 16 2017, 2:02 PM
davide accepted this revision.Nov 16 2017, 2:09 PM

LGTM with Peter comment's addressed (and please don't forget to switch this to a SmallSetVector ;)

dmikulin marked 5 inline comments as done.Nov 16 2017, 2:37 PM

And the set type is changed to SmallSetVector.

This revision was automatically updated to reflect the committed changes.