Page MenuHomePhabricator

[ARM] LoadStoreOptimizer: reoder limit
ClosedPublic

Authored by SjoerdMeijer on Fri, Feb 8, 8:41 AM.

Details

Summary

Today I was a bit distracted by the loadstoreoptimiser because it didn't do
what I wanted/expected.

This patch adds a (hidden) option to control the total number of instructions that
can be re-ordered. I appreciate this looks only a tiny bit better than a hard-coded
constant, but at least it allows more easy experimentation with different values
for now. Ideally we calculate this reorder limit based on some heuristics. I might be
looking into that next.

Second patch D57955 is just a simple clean up.

Diff Detail

Repository
rL LLVM

Event Timeline

SjoerdMeijer created this revision.Fri, Feb 8, 8:41 AM
SjoerdMeijer edited the summary of this revision. (Show Details)Fri, Feb 8, 8:45 AM
efriedma accepted this revision.Fri, Feb 8, 12:13 PM

I guess this is fine. LGTM

Ultimately, the whole design here is sort of flaky: the scheduling code here isn't aware of register pressure, and the register allocator isn't aware of the possibility of generating ldm/stm, so the end result is very unreliable. I'm not sure what the right solution is, but tweaking the scheduling heuristics isn't going to help much.

This revision is now accepted and ready to land.Fri, Feb 8, 12:13 PM

Thanks Eli, also for your thoughts on this:

Ultimately, the whole design here is sort of flaky: the scheduling code here isn't aware of register pressure, and the register allocator isn't aware of the possibility of generating ldm/stm, so the end result is very unreliable.

Unreliable results is exactly what I was seeing, and that's why I started studying first what exactly this loadstoreoptimiser is doing.

I'm not sure what the right solution is, but tweaking the scheduling heuristics isn't going to help much.

Agreed. Also sounds a little bit like a can of worms. I need to first investigate more where exactly my problems are caused, depending on that I might be looking more into this. Making this rescheduling register pressure aware might be a good step in the right direction? But as you also said, not the complete answer...

This revision was automatically updated to reflect the committed changes.
Herald added a project: Restricted Project. · View Herald TranscriptMon, Feb 11, 1:37 AM