Page MenuHomePhabricator

AArch64LoadStoreOptimizer: Update kill flags when merging stores
ClosedPublic

Authored by MatzeB on Jan 18 2017, 3:37 PM.

Details

Summary

Kill flags need to be updated correctly when moving stores up/down to
form store pair instructions.
Those invalid flags have been ignored before but as of r290014 they are
recognized when using -mllvm -verify-machineinstrs.

Also simplifies test/CodeGen/AArch64/ldst-opt-dbg-limit.mir, renames it
to ldst-opt.mir test and adds a new tests for this change.

Diff Detail

Repository
rL LLVM

Event Timeline

MatzeB created this revision.Jan 18 2017, 3:37 PM
junbuml added inline comments.Jan 19 2017, 8:49 AM
lib/Target/AArch64/AArch64LoadStoreOptimizer.cpp
695

As far as I see, only store pair seem to be the issue, but just want to confirm if there is any case for load pair ?

697

Don't we need to set kill on it ?

699–700

If there is no instruction using RegOp0 and RegOp1 between stores, no need to clear kill.

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

Don't we need to set kill on w1 here ?

MatzeB marked 4 inline comments as done.Jan 19 2017, 10:14 AM

Thanks for the review.

The thing with Kill flags is that we want to get rid of them long term. Today they are optional: The last use of a value may or may not have a kill flag set, but if there is a kill flag, then the value must be killed at that point.
This patch fixes situations in which invalid kill flags are produced, it can remove more kill flags than necessary or not adding possible new ones; But that is fine and keeps the code simpler.

lib/Target/AArch64/AArch64LoadStoreOptimizer.cpp
695

Paired loads are fine, the loaded values are defs and the base register is taken from the place where the final paired instruction will end up (const MachineOperand &BaseRegOp = MergeForward ? getLdStBaseOp(*Paired) : getLdStBaseOp(*I);) so the kill flags are correct for that.

697

Kills are optional (see above)

699–700

Yes there is no need, but it doesn't hurt either and keeps the code slightly simpler.

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

Kills are optional (see above).

mcrosier edited edge metadata.Jan 19 2017, 11:04 AM

Given we want to deprecate the use of kill flags and AFAIK there are no other passes that use these flags after the load/store optimizer, I'm okay with the lossiness of this solution.

lib/Target/AArch64/AArch64LoadStoreOptimizer.cpp
707

If the two instructions being paired happen to be adjacent to one another, NextI will point to the instruction after 'Paired'. Is this going to behave as expected with make_range()?

MatzeB updated this revision to Diff 85067.Jan 19 2017, 5:06 PM
MatzeB marked 4 inline comments as done.

Thanks for the review, address comment.

MatzeB marked an inline comment as done.Jan 19 2017, 5:06 PM
MatzeB added inline comments.
lib/Target/AArch64/AArch64LoadStoreOptimizer.cpp
707

Good point, for some reason that case never happened if MergeForward was true, but what I really should be used here is std::next(I).

mcrosier accepted this revision.Jan 20 2017, 7:01 AM

LGTM.

This revision is now accepted and ready to land.Jan 20 2017, 7:01 AM
This revision was automatically updated to reflect the committed changes.