This is an archive of the discontinued LLVM Phabricator instance.

[RegAllocGreedy] avoid using physreg candidates that cannot be correctly spilled
ClosedPublic

Authored by dfukalov on Sep 13 2018, 12:51 PM.

Details

Summary

For the AMDGPU target if a MBB contains exec mask restore preamble, SplitEditor may get state when it cannot insert a spill instruction.

E.g. for a MIR

bb.100:
    %1 = S_OR_SAVEEXEC_B64 %2, implicit-def $exec, implicit-def $scc, implicit $exec

and if the regalloc will try to allocate a virtreg to the physreg already assigned to virtreg %1, it should insert spill instruction before the S_OR_SAVEEXEC_B64 instruction.
But it is not possible since can generate incorrect code in terms of exec mask.

The change makes regalloc to ignore such physreg candidates.

Diff Detail

Event Timeline

dfukalov created this revision.Sep 13 2018, 12:51 PM
rampitec added inline comments.Sep 13 2018, 1:08 PM
test/CodeGen/AMDGPU/spill-before-exec.mir
7

This test will not work in a non-debug build. It needs at least

; REQUIRES: asserts

Hi,

Could you elaborate on that part?

But it is not possible since can generate incorrect code in terms of exec mask.

I don't understand the constraints and thus what you are trying to achieve.

Cheers,
-Quentin

dfukalov updated this revision to Diff 165532.Sep 14 2018, 10:37 AM

Thanks! Looks good, but please wait for others to review.

dfukalov marked an inline comment as done.Sep 14 2018, 10:50 AM

Hi,

Could you elaborate on that part?

But it is not possible since can generate incorrect code in terms of exec mask.

I don't understand the constraints and thus what you are trying to achieve.

Cheers,
-Quentin

SplitEditor during splitting region can try to insert COPY spill instruction before S_OR_SAVEEXEC_B64 since it interferes with physreg candidate that is already assigned to destination of this exec mask restoration instruction. Unfortunately, such a COPY operation may be lowered to a memory operation that will not be correct because exec mask is not restored before the preamble.

There was related fix https://reviews.llvm.org/D27997 for MachineBasicBlock::SkipPHIsLabelsAndDebug to skip exec masks restore preamble.

test/CodeGen/AMDGPU/spill-before-exec.mir
7

thanks!

Hi,

Could you elaborate on the exact constraints?

My understanding is that you don't want split code to occur right before this instruction, but I would like to understand exactly what it is not allowed.

Indeed, it would be easy to trick your fix by building an artificial basic block just before that instruction, so it would be good to go back to the constraints and see how we can properly encode them.

Cheers,
-Quentin

dfukalov marked an inline comment as done.Sep 17 2018, 12:57 PM

My understanding is that you don't want split code to occur right before this instruction, but I would like to understand exactly what it is not allowed.

The amdgpu target has special exec mask register that specifies what lanes should execute current vector instruction. So a basic block created from "if/else" construction with vector condition may contain this exec mask register restore code in preamble.
E.g. instruction S_OR_SAVEEXEC_B64 at the start of a BB set (restores) exec mask register value to correctly execute "else" block. So a spilling code SplitKit tries to insert before such an instruction may be transformed into memory operation. And it can be incorrectly executed with a wrong exec mask.

Indeed, it would be easy to trick your fix by building an artificial basic block just before that instruction, so it would be good to go back to the constraints and see how we can properly encode them.

Such new BB should take into account that "effective CFG" of vector operations depends on exec mask register value.

This revision is now accepted and ready to land.Sep 24 2018, 9:11 AM
This revision was automatically updated to reflect the committed changes.