This is an archive of the discontinued LLVM Phabricator instance.

Re-land MachineInstr: Reason locally about some memory objects before going to AA.
ClosedPublic

Authored by bmakam on Aug 18 2017, 2:39 PM.

Details

Summary

Reverts r311008 to reinstate r310825 with a fix.

Refine alias checking for pseudo vs value to be conservative.
This fixes the original failure in builtbot unittest SingleSource/UnitTests/2003-07-09-SignedArgs.

Diff Detail

Event Timeline

bmakam created this revision.Aug 18 2017, 2:39 PM
bjope added a subscriber: bjope.Aug 19 2017, 8:03 AM
bmakam updated this revision to Diff 111902.Aug 20 2017, 1:33 PM
bmakam edited the summary of this revision. (Show Details)
bmakam added a reviewer: nemanjai.

Trim the patch and avoid handling stack references for non-value MMOs to prevent LNT test failures.

bmakam updated this revision to Diff 112076.Aug 21 2017, 3:49 PM
bmakam retitled this revision from [WIP] Re-land MachineInstr: Reason locally about some memory objects before going to AA. to Re-land MachineInstr: Reason locally about some memory objects before going to AA..
bmakam edited the summary of this revision. (Show Details)
bmakam updated this revision to Diff 112608.Aug 24 2017, 1:53 PM
bmakam updated this revision to Diff 113111.Aug 29 2017, 10:31 AM

rebase and fix x86 lit test.

bjope added inline comments.Aug 29 2017, 11:20 AM
lib/CodeGen/MachineInstr.cpp
1703

These asserts are related to the interface to AA, right? (I think it says so in the comments above)

Are they also important for the new new checks you have added, or could/should the asserts perhaps be moved closer to the AA->alias call?
Now it kind of looks like the asserts are guarding your new checks, but the comments are referring to assumptions made by the API to AA.

(Some more background to why I'm asking:)
Our out-of-tree target is actually creating StackPointer relative MachingMemOperands with negative offsets. In the past we never hit those asserts since the ealier (!MMOa->getValue() || !MMOb->getValue()) check would make us return before checking for negative operands.
I do not think that we need those negative offsets, so I'll probably try to get rid of them regardless of this patch. But as long as you put the asserts before the "if (!ValA || !ValB)", then we must stop using negative offsets in PseudoSourceValue kind of MachingMemOperands or we will hit the assert.

bmakam updated this revision to Diff 113160.Aug 29 2017, 2:39 PM
bmakam marked an inline comment as done.
bmakam added a reviewer: efriedma.

Move assert closer to AA->alias call.

efriedma accepted this revision.Aug 29 2017, 5:44 PM

Makes sense.

lib/CodeGen/MachineInstr.cpp
1713

Indentation?

This revision is now accepted and ready to land.Aug 29 2017, 5:44 PM
This revision was automatically updated to reflect the committed changes.
bmakam marked an inline comment as not done.
bmakam marked 2 inline comments as done.Aug 30 2017, 8:05 AM

Thanks for the review.