This is an archive of the discontinued LLVM Phabricator instance.

[InstCombine] Don't let an alignment assume prevent new/delete removals.
ClosedPublic

Authored by hjyamauchi on Jun 15 2020, 10:25 AM.

Details

Diff Detail

Event Timeline

hjyamauchi created this revision.Jun 15 2020, 10:25 AM
Herald added a project: Restricted Project. · View Herald TranscriptJun 15 2020, 10:25 AM
Herald added a subscriber: hiraditya. · View Herald Transcript

Can this be generalized to all assumes? nor just the code pattern that checks alignment?

This is really a temporary workaround until bundle representation of assumptions is used.
It might be better to work on that instead
See also D75462.

jdoerfert added a comment.EditedJun 16 2020, 1:48 PM

This is the right fix for alignment: D71739

For all assumes (and later other stuff, think lifetime markers or annotations) you can check if the User::isDroppable, and eventually drop via Value::dropDroppableUses if you cannot update them properly.

This is the right fix for alignment: D71739

For all assumes (and later other stuff, think lifetime markers or annotations) you can check if the User::isDroppable, and eventually drop via Value::dropDroppableUses if you cannot update them properly.

@lebedev.ri, @jdoerfert: I acknowledge D71739. What's the status of it? Are you open to having this patch as a temporary measure until that's checked in?

perhaps add a FIXME comment to point to the related patch and final state.

In D81854#2098885, @yamauchi wrote:

This is the right fix for alignment: D71739

For all assumes (and later other stuff, think lifetime markers or annotations) you can check if the User::isDroppable, and eventually drop via Value::dropDroppableUses if you cannot update them properly.

@lebedev.ri, @jdoerfert: I acknowledge D71739. What's the status of it? Are you open to having this patch as a temporary measure until that's checked in?

I pinged the other patch. It is basically ready, minor changes only. Let's wait a day or so and see if @Tyker responds.

@Tyker merged D71739, if it sticks, can you check if this is still necessary?

@Tyker merged D71739, if it sticks, can you check if this is still necessary?

Thanks. This is still necessary for this particular alloc removal case, but D71739 made it much simpler.

This revision is now accepted and ready to land.Jun 26 2020, 10:59 AM
jdoerfert accepted this revision.Jun 26 2020, 1:57 PM

Looks much better indeed ;)

Please add a commit message.

hjyamauchi updated this revision to Diff 274180.EditedJun 29 2020, 10:51 AM

Update a test which was somehow missed.

hjyamauchi edited the summary of this revision. (Show Details)Jun 29 2020, 10:51 AM

PTAL.

davidxl added inline comments.Jun 29 2020, 12:02 PM
llvm/test/Transforms/PhaseOrdering/inlining-alignment-assumptions.ll
101

what is this change about?

hjyamauchi marked an inline comment as done.Jun 29 2020, 12:35 PM
hjyamauchi added inline comments.
llvm/test/Transforms/PhaseOrdering/inlining-alignment-assumptions.ll
101

This is another example that this patch improves (fixing a problem that an assume was preventing the removal of an alloca).

davidxl added inline comments.Jun 29 2020, 1:13 PM
llvm/test/Transforms/PhaseOrdering/inlining-alignment-assumptions.ll
88

This comment is out of date then

hjyamauchi marked an inline comment as done.

Address comment.

llvm/test/Transforms/PhaseOrdering/inlining-alignment-assumptions.ll
88

Updated.

This revision was automatically updated to reflect the committed changes.