Page MenuHomePhabricator

[X86] Lower frame references with large offsets
Needs ReviewPublic

Authored by greened on Oct 28 2019, 2:34 PM.



Handle offsets > 32 bits by using the register scavenger to find a register to
hold the large offset and change the frame index reference to use SIB

Diff Detail

Event Timeline

greened created this revision.Oct 28 2019, 2:34 PM
Herald added a project: Restricted Project. · View Herald TranscriptOct 28 2019, 2:34 PM

This likely needs some work. I wanted to get it up early because it includes a rather significant change, the first use of register scavenging in the X86 backend as far as I'm aware. If there is a better way to do this I'm all ears.

This causes a couple of tests to fail due to the X86 backend not keeping liveness information up-to-date. My understanding is that the liveness bits should go away. Currently PEI uses RegisterScavenging::forward which is commented as being not preferred. The better option is to use RegisterScavenging::backward which doesn't need to rely on liveness information. However, PEI iterates through the basic block in a forward manner, so using RegisterScavenging::backward could be expensive.

Other targets like AArch64 use register scavenging so they are already relying on an interface that is essentially marked deprecated. What should be the solution here? Fixing up the couple of places in the X86 codegen to properly update liveness isn't too bad but I wonder about other lurking issues. Is it time to rework PEI so that RegisterScavenging::backward can be used?

greened updated this revision to Diff 226971.Oct 29 2019, 2:33 PM

This revision avoids the more general use of register scavenging, opting instead for a local register scavenger which can operate in backward mode, which doesn't need up-to-date liveness information. This seems safer to me, avoiding lots of potentially unknown pitfalls in the X86 backend.

I am not sure whether the FIXME actually applies. If the scavenger is guaranteed to return a stack slot with a small offset, then we can lift the spilling restriction.

arsenm added inline comments.Oct 31 2019, 3:44 PM

I think this is redundant with passing the iterator to scavengeRegisterBackwards but I could be misremembering

greened marked 2 inline comments as done.Nov 1 2019, 10:19 AM
greened added inline comments.

It's actually not. I thought the same but the way scavengeRegisterBackward works is it tries to find a free register over the entire range of the current position and the iterator passed to it. So if you don't call backward() ahead of time you end up saying you require a register free from the passed iterator to the end of the block, which is not likely to be available. :) . I found that out the hard way.

greened marked an inline comment as done.Nov 1 2019, 10:20 AM
arsenm added inline comments.Jan 9 2020, 2:34 PM

These can be dropped


Should be moved to -mtriple argument


Use named values


Attributes can probably be removed