Page MenuHomePhabricator

[AMDGPU] Split 64-Bit XNOR to 64-Bit NOT/XOR
AcceptedPublic

Authored by grahamsellers on Thu, Nov 29, 12:12 PM.

Details

Reviewers
arsenm
nhaehnle
Summary

The identity ~(x ^ y) == (~x ^ y) == (x ^ ~y) allows XNOR (XOR/NOT) to turn into NOT/XOR. Handling this case with its own split means we can make the NOT remain in the scalar unit. Previously, we split 64-bit XNOR into two 32-bit XNOR, then lowered. Now, we get three instructions (s_not, v_xor, v_xor) rather than four in the case where either of the sources is a scalar 64-bit.

Add test cases to xnor.ll to attempt XNOR Vx, Sy and XNOR Sx, Vy. Also adding test that uses the opposite identity such that (~x ^ y) on the scalar unit (or vector for gfx906) can generate XNOR. This already worked, but I didn't see a test for it.

Diff Detail

Event Timeline

grahamsellers created this revision.Thu, Nov 29, 12:12 PM
arsenm added inline comments.Thu, Nov 29, 12:24 PM
lib/Target/AMDGPU/SIInstrInfo.cpp
4908

const reference

4927

.add on next line

test/CodeGen/AMDGPU/xnor.ll
128

It would be good if you can include some other use that requires SALU->VALU changes to stress that the wordlist insert happened

Addressing review.

grahamsellers marked 3 inline comments as done.Thu, Nov 29, 12:43 PM
grahamsellers added inline comments.
lib/Target/AMDGPU/SIInstrInfo.cpp
4908

I stole this from elsewhere in this file. I went ahead and fixed other places where this was not const reference.

test/CodeGen/AMDGPU/xnor.ll
128

Not sure I follow. The new split turns S_XNOR_B64 into S_NOT_B64 followed by S_XOR_B64, adding the S_XOR_B64 to the worklist. The only way the two V_XOR_B32 instructions can be generated is when SIInstrInfo::moveToVALU iterates again, converting S_XOR_B64 -> 2 x S_XOR_B32, which re-adds to the worklist, causing moveToVALU to turn S_XOR_B32 to V_XOR_B32_E32 (which the test expects). If the worklist insert wasn't happening, none of these tests would work.

What am I missing?

arsenm accepted this revision.Thu, Nov 29, 2:21 PM

LGTM

lib/Target/AMDGPU/SIInstrInfo.cpp
4908

The guidance used to be to use values, but then that changed at some point but nobody ever went back and changed them all

This revision is now accepted and ready to land.Thu, Nov 29, 2:21 PM