This is an archive of the discontinued LLVM Phabricator instance.

AMDGPU: Allow additional implicit operands on MOVRELS instructions
ClosedPublic

Authored by nhaehnle on Oct 16 2016, 6:14 AM.

Details

Summary

The post-RA scheduler occasionally uses additional implicit operands when
the vector implicit operand as a whole is killed, but some subregisters
are still live because they are directly referenced later. Unfortunately,
this seems incredibly subtle to reproduce.

Fixes piglit spec/glsl-110/execution/variable-indexing/vs-temp-array-mat2-index-wr.shader_test
and others.

Diff Detail

Repository
rL LLVM

Event Timeline

nhaehnle updated this revision to Diff 74793.Oct 16 2016, 6:14 AM
nhaehnle retitled this revision from to AMDGPU: Allow additional implicit operands on MOVRELS instructions.
nhaehnle updated this object.
nhaehnle added reviewers: arsenm, tstellarAMD.
nhaehnle added a subscriber: llvm-commits.
arsenm edited edge metadata.Oct 16 2016, 10:00 AM

Can you add a test for this?

nhaehnle updated this revision to Diff 74982.Oct 18 2016, 5:06 AM
nhaehnle edited edge metadata.

Adding a test case.

The test case is quite fragile because the problem comes from
ScheduleDAGInstrs::fixupKills, which seems to have a bunch of subtle
problems with sub-registers, and generally the code just doesn't really
do what the comments seem to think it does. But fixing that may not be
worth it since the whole thing has a FIXME on it suggesting to switch it
all to LivePhysRegs.

Adding a test case.

The test case is quite fragile because the problem comes from
ScheduleDAGInstrs::fixupKills, which seems to have a bunch of subtle
problems with sub-registers, and generally the code just doesn't really
do what the comments seem to think it does. But fixing that may not be
worth it since the whole thing has a FIXME on it suggesting to switch it
all to LivePhysRegs.

Would it be easier to write an MIR tests case?

nhaehnle updated this revision to Diff 75580.Oct 24 2016, 7:53 AM
nhaehnle edited edge metadata.

Use a MIR test instead.

tstellarAMD accepted this revision.Oct 27 2016, 4:12 PM
tstellarAMD edited edge metadata.

LGTM.

This revision is now accepted and ready to land.Oct 27 2016, 4:12 PM
This revision was automatically updated to reflect the committed changes.