Page MenuHomePhabricator

[MemorySSAUpdater] Avoid creating self-referencing MemoryDefs
ClosedPublic

Authored by labrinea on Sep 7 2018, 10:36 AM.

Details

Summary

Fix for https://bugs.llvm.org/show_bug.cgi?id=38807, which occurred while compiling SemaTemplateInstantiate.cpp with clang and GVNHoist enabled.
In the following example:

      1=def(entry)
      /        \
2=def(1)       4=def(1)
3=def(2)       5=def(4)

When removing the MemoryDef 2=def(1) from its basic block, and just before adding it to the end of the parent basic block, we first replace all its uses with the defining memory access:

3=def(2) -> 3=def(1)

Then we call insertDef for adding 2=def(1) to the parent basic block, where we replace the uses of 1=def(entry) with 2=def(1). Doing so we create a self reference:

2=def(1) -> 2=def(2)  (bad)
3=def(1) -> 3=def(2)  (ok)
4=def(1) -> 4=def(2)  (ok)

Diff Detail

Repository
rL LLVM

Event Timeline

labrinea created this revision.Sep 7 2018, 10:36 AM
labrinea added inline comments.Sep 7 2018, 10:47 AM
lib/Analysis/MemorySSAUpdater.cpp
276 ↗(On Diff #164450)

Removing the comment as this is already happening and it's what we are trying to fix.

406 ↗(On Diff #164450)

Another way to fix this would be to add What->replaceUsesOfWith(What->getDefiningAccess(), nullptr); here, so that What gets removed from the Uses of its defining memory access. This would make sure that the graph of Uses/Users is in a correct state before calling insertDef(). However I haven't tested it thoroughly and it's also more expensive so I am in favor of the current fix.

test/Transforms/GVNHoist/pr38807.ll
32 ↗(On Diff #164450)

I am not sure why but the gep remains after the hoist, and goes away with dead code elimination. Is that correct @sebpop ?

42 ↗(On Diff #164450)

Same here.

sebpop accepted this revision.Sep 10 2018, 10:03 AM

Looks good to me.

test/Transforms/GVNHoist/pr38807.ll
32 ↗(On Diff #164450)

gvn hoist does not need to implement a dce.

This revision is now accepted and ready to land.Sep 10 2018, 10:03 AM
This revision was automatically updated to reflect the committed changes.