This is an archive of the discontinued LLVM Phabricator instance.

MCP: Fixed bug with dest overlapping copy source
ClosedPublic

Authored by tpr on Nov 7 2019, 9:37 AM.

Details

Summary

In MachineCopyPropagation, when propagating the source of a copy into
the operand of a later instruction, bail if a destination overlaps the
copy source. If the instruction where the substitution is happening is
also a copy, allowing the propagation confuses the tracking mechanism.

Event Timeline

tpr created this revision.Nov 7 2019, 9:37 AM
tpr added a subscriber: arsenm.
arsenm added inline comments.Nov 7 2019, 10:57 AM
llvm/lib/CodeGen/MachineCopyPropagation.cpp
439–442

This will miss implicit defs. Should be checking MI.modifiesRegister maybe?

llvm/test/CodeGen/AMDGPU/mcp-overlap-after-propagation.mir
21

Should add another case where the relevant def is an implicit def

lkail added a subscriber: lkail.Nov 7 2019, 2:54 PM
foad added a subscriber: foad.Nov 8 2019, 4:19 AM
tpr updated this revision to Diff 228461.Nov 8 2019, 8:13 AM

V2: Used modifiesRegister as suggested by Matt.

tpr marked 3 inline comments as done.Nov 8 2019, 8:16 AM
tpr added inline comments.
llvm/lib/CodeGen/MachineCopyPropagation.cpp
439–442

Good idea. Makes the code simpler too.

llvm/test/CodeGen/AMDGPU/mcp-overlap-after-propagation.mir
21

I don't think I can test that. The bug only actually has an effect if the instruction that the copy is being folded into is itself a copy, and I can't think of a way then of there being an implicit def that overlaps the source of the first copy.

arsenm accepted this revision.Nov 8 2019, 2:47 PM

LGTM

llvm/lib/CodeGen/MachineCopyPropagation.cpp
440

I'm pretty sure the << MI already includes the newline, so you can drop the extra one here

This revision is now accepted and ready to land.Nov 8 2019, 2:47 PM
tpr updated this revision to Diff 228684.Nov 11 2019, 6:37 AM
tpr marked an inline comment as done.

V3: Only bail if it is a copy and a partial def, to avoid spurious test changes.

tpr marked an inline comment as done.Nov 11 2019, 12:41 PM
This revision was automatically updated to reflect the committed changes.
arsenm added inline comments.Nov 12 2019, 2:02 AM
llvm/lib/CodeGen/MachineCopyPropagation.cpp
438–439

Maybe should have a partiallyModifiesRegister?