This is an archive of the discontinued LLVM Phabricator instance.

[Mem2Reg] Teach promote to register about droppable instructions
ClosedPublic

Authored by jdoerfert on Jul 16 2020, 12:00 PM.

Details

Summary

This is the first of two patches to address PR46753. We basically allow
mem2reg to promote allocas that are used in doppable instructions, for
now that means llvm.assume. The uses of the alloca (or a bitcast or
zero offset GEP from there) are replaced by undef in the droppable
instructions.

Diff Detail

Event Timeline

jdoerfert created this revision.Jul 16 2020, 12:00 PM
Herald added a project: Restricted Project. · View Herald TranscriptJul 16 2020, 12:00 PM

Remove code duplication

thank you for working on this

llvm/lib/Transforms/Utils/PromoteMemoryToRegister.cpp
347

is there a reason you are not using dropDroppableUses here.

replacing uses in an assume by undef is usually not a safe transformation. perhaps there is UB is not exploited for operand bundles but it is very likely to be exploited for the usual boolean argument of assume.

nikic added a subscriber: nikic.Jul 16 2020, 12:21 PM
jdoerfert marked an inline comment as done.

Use proper function to drop uses

Tyker accepted this revision.Jul 20 2020, 11:25 AM

just one nit

llvm/lib/Transforms/Utils/PromoteMemoryToRegister.cpp
324

using dropDroppableUses this way is a bit weird since it is already doing a traversal through the uses but it is correct.

maybe an API to drop a single use would be appropriate.

This revision is now accepted and ready to land.Jul 20 2020, 11:25 AM
jdoerfert marked an inline comment as done.Jul 20 2020, 4:03 PM
jdoerfert added inline comments.
llvm/lib/Transforms/Utils/PromoteMemoryToRegister.cpp
324

I'll look into it and update this. I'll commit it if it seems straight forward, post review if anyone disagrees.

jdoerfert marked an inline comment as done.

Introduce API to drop user uses

efriedma added inline comments.
llvm/lib/IR/Value.cpp
197 ↗(On Diff #280542)

This seems like a dubious pattern; you're iterating over the entire use list of the value each time you call dropDroppableUsesByUser, which is likely a lot more expensive than iterating over the operand list of the assume.

fhahn added inline comments.Jul 27 2020, 5:46 AM
llvm/lib/IR/Value.cpp
197 ↗(On Diff #280542)

@jdoerfert @Tyker Any more thoughts on Eli's point?

jdoerfert marked an inline comment as done.Jul 27 2020, 7:02 AM

(I missed the comment, apologies)

llvm/lib/IR/Value.cpp
197 ↗(On Diff #280542)

I have no particular preference. Unclear what is better (over time). I'll propose a follow up patch that uses the assume operands instead.

efriedma added inline comments.Jul 27 2020, 11:55 AM
llvm/lib/IR/Value.cpp
197 ↗(On Diff #280542)

To be precise, my concern is that the overall algorithm is O(N^2) over the number of uses of the alloca.

jdoerfert added inline comments.Jul 28 2020, 2:58 PM
llvm/lib/IR/Value.cpp
197 ↗(On Diff #280542)

D84804 should address this.

Tyker added inline comments.Jul 28 2020, 3:24 PM
llvm/lib/IR/Value.cpp
197 ↗(On Diff #280542)

i agree with Eli, i tried express my concern about that in https://reviews.llvm.org/D83976?id=280542#inline-775547