This is an archive of the discontinued LLVM Phabricator instance.

[Tail duplication] Handle source registers with subregisters
ClosedPublic

Authored by kparzysz on Apr 20 2016, 11:46 AM.

Details

Summary

When a block is tail-duplicated, the PHI nodes from that block are replaced with appropriate COPY instructions. When those PHI nodes contained use operands with subregisters, the subregisters were dropped from the COPY instructions, resulting in incorrect code.

Keep track of the subregister information and use this information when remapping instructions from the duplicated block.

Diff Detail

Repository
rL LLVM

Event Timeline

kparzysz updated this revision to Diff 54397.Apr 20 2016, 11:46 AM
kparzysz retitled this revision from to [Tail duplication] Handle source registers with subregisters.
kparzysz updated this object.
kparzysz added a reviewer: MatzeB.
kparzysz set the repository for this revision to rL LLVM.
kparzysz added a subscriber: llvm-commits.
qcolombet requested changes to this revision.Apr 25 2016, 3:01 PM
qcolombet added a reviewer: qcolombet.
qcolombet added a subscriber: qcolombet.

Hi Krzysztof,

This looks mostly good to me. The change on how we add register class constrains seems broken to me and the test case could benefit some check lines and a run of opt -instnamer.

Cheers,
-Quentin

lib/CodeGen/TailDuplicator.cpp
366

Why do you check for the subreg here?
If you believe the adding constraints will not work, you should check the result of constrainRegClass and act accordingly.

877

Use \p in front of variable names.

test/CodeGen/Hexagon/tail-dup-subreg-map.ll
1

FileCheck?

This revision now requires changes to proceed.Apr 25 2016, 3:01 PM
kparzysz added inline comments.Apr 26 2016, 8:32 AM
lib/CodeGen/TailDuplicator.cpp
366

This is because we are doing a replacement

Reg -> VI.Reg:VI.SubReg

If SubReg is present, then VI.Reg belongs to a class where its sub-registers are compatible with Reg, but VI.Reg itself does not have to be. Actually, I think that the existing code is broken.

kparzysz updated this revision to Diff 55010.Apr 26 2016, 8:33 AM
kparzysz edited edge metadata.

Expanded handling of the register class constraints when remapping TailBB into its predecessor.

kparzysz added inline comments.Apr 26 2016, 8:35 AM
lib/CodeGen/TailDuplicator.cpp
877

Sorry, missed this one.

kparzysz updated this revision to Diff 55011.Apr 26 2016, 8:37 AM
kparzysz edited edge metadata.

Fixed a comment.

qcolombet added inline comments.Apr 26 2016, 9:13 AM
lib/CodeGen/TailDuplicator.cpp
373

MRI->setRegClass.

389

Instead of using OrigRC, we should use the regclass that is required by MO.
MachineInstr::getRegClassConstraint

392

We need to read from Reg:subReg if Reg has a sub reg.

kparzysz marked 3 inline comments as done.Apr 26 2016, 11:16 AM
kparzysz added inline comments.
lib/CodeGen/TailDuplicator.cpp
392

Actually, we need to leave the sub-reg alone. NewReg will be inserted into the LocalVRMap as a replacement for Reg. Since there is no Reg anymore (it should be the value taken from VI.Reg:VI:SubReg), the COPY should use the VI.* values instead of Reg.

kparzysz updated this revision to Diff 55048.Apr 26 2016, 11:17 AM

Update the instruction remapping code.

qcolombet accepted this revision.Apr 26 2016, 11:22 AM
qcolombet edited edge metadata.

Hi Krzysztof,

LGTM.

Thanks,
-Quentin

This revision is now accepted and ready to land.Apr 26 2016, 11:22 AM
This revision was automatically updated to reflect the committed changes.