This is an archive of the discontinued LLVM Phabricator instance.

[AArch64] Preserve register flags when promoting a load from store.
ClosedPublic

Authored by fhahn on Jun 20 2017, 7:10 AM.

Details

Summary

This patch updates promoteLoadFromStore to use the store MachineOperand as the
source operand of the of the new instruction instead of creating a new
register MachineOperand. This way, the existing register flags are
preserved.

This fixes PR33468 (https://bugs.llvm.org/show_bug.cgi?id=33468).

Diff Detail

Event Timeline

fhahn created this revision.Jun 20 2017, 7:10 AM
fhahn edited the summary of this revision. (Show Details)Jun 20 2017, 7:10 AM
t.p.northover edited edge metadata.Jun 20 2017, 7:26 AM

I'm not sure that's valid. It seem like we'd still be broken in this situation:

STR %val, %base, ...
[...]
SOMETHING %val<killed>, ...
[...]
%whatever = LDR %base

findMatchingStore looks like it'd allow this (%val isn't actually changed) but the liveness is still broken after replacing the LDR with a MOV.

fhahn added a comment.Jun 20 2017, 7:31 AM

Agreed. It seems we have to keep track of the flags for all instructions using the store operand (between the store and load) and use that when creating the promoted operand. findMachtingStore would be a good place to do that, no?

I'm not sure findMatchingStore can do it easily. During its reverse scan it doesn't know what the value register is going to be and keeping track of the most recent kill of all registers seems a little excessive.

I think the code clearing kill flags later on (~line 877) probably has to have a reference to the MachineOperand and set its kill flag if necessary.

junbuml edited edge metadata.Jun 20 2017, 8:02 AM

I think the code clearing kill flags later on (~line 877) probably has to have a reference to the MachineOperand and set its kill flag if necessary.

+1

fhahn updated this revision to Diff 103220.Jun 20 2017, 9:24 AM
fhahn edited the summary of this revision. (Show Details)

Thanks, that looks like an excellent place! Updated the patch to propagate the kill flag to the promoted operand.

junbuml added inline comments.Jun 20 2017, 9:52 AM
lib/Target/AArch64/AArch64LoadStoreOptimizer.cpp
885

Isn't it okay to do break after this ?

fhahn added inline comments.Jun 20 2017, 10:01 AM
lib/Target/AArch64/AArch64LoadStoreOptimizer.cpp
885

Yep it should be OK, assuming there are no redundant kills later on, but then it's not the responsibility of this pass to clean that up.

fhahn updated this revision to Diff 103230.Jun 20 2017, 10:12 AM

Added an early exit after the first kill flag has been found.

MatzeB edited edge metadata.Jun 20 2017, 11:10 AM

The first part of the patch looks fine to me.

The second part of adding an extra kill flag should be unnecessary and I'd prefer staying with the naive code:

  • Kill flags are optional. So not adding them should never be a correctness problem.
  • Missing kill flags may mean missed optimizations today, though I believe that I fixed all passes used by AArch64 in that respect so there should be no change. (Admittedly I still have to do some larger scaling testing with a debugging mode that drops all kill flags to verify it is true).
  • The long term plan is to get rid of kill flags and hence having simpler code.
test/CodeGen/AArch64/ldst-opt.mir
153

Is it possible to make %w1 not live-in to make the test slightly more obvious.

Ok, I'll update the patch and remove the code to propagate the kill flag. Do you think it's worth to keep the early exit at line 887 though?

test/CodeGen/AArch64/ldst-opt.mir
153

Yes I'll do that

Ok, I'll update the patch and remove the code to propagate the kill flag. Do you think it's worth to keep the early exit at line 887 though?

If you want to. Though may be a good idea to put it into a separate commit (without an extra review).

fhahn updated this revision to Diff 103246.Jun 20 2017, 11:53 AM

Undo changes to promote kill flag, early exit and removed %w1 from liveins

fhahn edited the summary of this revision. (Show Details)Jun 20 2017, 11:53 AM
fhahn marked an inline comment as done.
MatzeB accepted this revision.Jun 20 2017, 12:03 PM

Thanks, LGTM

This revision is now accepted and ready to land.Jun 20 2017, 12:03 PM
fhahn closed this revision.Jun 21 2017, 1:48 AM