Page MenuHomePhabricator

[ARM] ARMLoadStoreOpt::UpdateBaseRegUses should stop on def
ClosedPublic

Authored by john.brawn on Jun 19 2015, 7:10 AM.

Details

Summary

When UpdateBaseRegUses sees an instruction that defines the base register it must stop, as the base register value it is updating is no longer live. Ideally we would already have seen the register be killed (which is already checked for), but the kill flags may be inaccurate and we have to account for this.

Diff Detail

Repository
rL LLVM

Event Timeline

john.brawn updated this revision to Diff 28021.Jun 19 2015, 7:10 AM
john.brawn retitled this revision from to [ARM] ARMLoadStoreOpt::UpdateBaseRegUses should stop on def.
john.brawn updated this object.
john.brawn edited the test plan for this revision. (Show Details)
john.brawn added reviewers: MatzeB, rengolin.
john.brawn set the repository for this revision to rL LLVM.
john.brawn added a subscriber: Unknown Object (MLST).
MatzeB edited edge metadata.Jun 19 2015, 2:47 PM

Nice catch! Kill flags are indeed optional nowadays so the additional check for definesRegister is necessary. Do you happen to have a llvm-lit testcase that exposes the bug?

Nice catch! Kill flags are indeed optional nowadays so the additional check for definesRegister is necessary. Do you happen to have a llvm-lit testcase that exposes the bug?

I have a .ll file that exposes the bug but it's ~150 lines long and generates assembly that's ~450 lines long, with the incorrectly-generated code sequence in the middle which is rather infeasible to test. I haven't been able to reduce it to produce a sensible test case - it looks like it's a very specific combination of regalloc and post-regalloc optimization that produces the input to the load/store optimizer that triggers the bug.

MatzeB accepted this revision.Jun 22 2015, 4:20 PM
MatzeB edited edge metadata.

It's probably tricky to get the compiler into a state where the kill flag is omitted. I guess until we have a way to serialize MI it is very hard to a proper testcase. So LGTM without testcase.

This revision is now accepted and ready to land.Jun 22 2015, 4:20 PM
This revision was automatically updated to reflect the committed changes.