Page MenuHomePhabricator

Revert "AMDGPU: Try to commute sub of boolean ext"
ClosedPublic

Authored by tpr on Dec 3 2019, 12:57 PM.

Details

Summary

This reverts commit 69fcfb7d3597e0cdb5554b4e672e9032b411b167.

As shown in the test I attached to this commit, the change I reverted
causes a problem with "zext(cc1) - zext(cc2)". It commuted
the operands to the sub and used different logic to select the addc/subc
instruction:

sub zext (setcc), x => addcarry 0, x, setcc
sub sext (setcc), x => subcarry 0, x, setcc

... but that is bogus. I believe it is not possible to fold those commuted
patterns into any form of addcarry or subcarry. It may have worked as
intended before "AMDGPU: Change boolean content type to 0 or 1" because
the setcc was considered to be -1 rather than 1.

Change-Id: If2139421aa6c935cbd1d925af58fe4a4aa9e8f43

Diff Detail

Event Timeline

tpr created this revision.Dec 3 2019, 12:57 PM
Herald added a project: Restricted Project. · View Herald TranscriptDec 3 2019, 12:57 PM
tpr updated this revision to Diff 232047.Dec 4 2019, 12:40 AM

V2: Removed an existing test for the bogus fold.

arsenm added inline comments.Dec 5 2019, 12:41 AM
llvm/test/CodeGen/AMDGPU/combine-cond-add-sub.ll
141

I think leaving the broken tests with the new emitted code would be more helpful than just deleting them

FWIW, this fixes a regression that introduced important glitches with Dead Rising 4 with both RADV/AMDVLK. Can this be pushed?

tpr added a comment.Dec 12 2019, 2:11 AM

Yes, sorry, I will do the test changes that Matt suggested so this can be approved and landed.

tpr updated this revision to Diff 233757.Dec 13 2019, 2:33 AM
V3: Reinstated and fixed the removed test.
tpr marked an inline comment as done.Dec 13 2019, 2:33 AM
arsenm accepted this revision.Dec 13 2019, 3:44 AM
This revision is now accepted and ready to land.Dec 13 2019, 3:44 AM
This revision was automatically updated to reflect the committed changes.