This is an archive of the discontinued LLVM Phabricator instance.

AMDGPU: Use -1/0 when copying from SCC to SGPR
AbandonedPublic

Authored by ruiling on Sep 14 2021, 4:56 AM.

Details

Summary

This is used to fix wrong code generation of s_add_co_select_user in
test/CodeGen/AMDGPU/expand-scalar-carry-out-select-user.ll

s_addc_u32 s4, s6, 0
s_cselect_b64 vcc, 1, 0    <-- vcc set as 0x1 if SCC==1
v_mov_b32_e32 v1, s4
s_cmp_gt_u32 s6, 31
v_cndmask_b32_e32 v1, 0, v1, vcc

If the s_addc_u32 set SCC, then we will get value 0x1 in VCC.
The v_cndmask will do per thread selection with VCC as condition
register. As VCC only gets the first bit being set, only the first
thread/lane in destination register v1 can get correct result. Other
lanes will not get correct result. And if the whole piece of code was
placed inside control flow, the first lane may be even inactive.
Setting all bits of VCC will make the v_cndmask work correctly,
and the result VGPR will have the value broadcasted to all active lanes.

The idea here is we set -1 for uniform booleans holding in SGPR, this
would allow direct logical operation with divergent boolean or directly
be used by vector instruction like in above case.

The other tests changes introduced by this patch seems functionally
correct to me.

Diff Detail

Event Timeline

ruiling created this revision.Sep 14 2021, 4:56 AM
ruiling requested review of this revision.Sep 14 2021, 4:56 AM
Herald added a project: Restricted Project. · View Herald TranscriptSep 14 2021, 4:56 AM
piotr added a comment.Sep 14 2021, 5:34 AM

The change makes sense to me in general.

When I tried doing the same long time ago, Matt rightly pointed out (https://reviews.llvm.org/D81925#inline-753225) that sign extending does not correspond to the booleans being defined as zero extended. So, wouldn't your change also require an accompanying change of boolean contents to ZeroOrNegativeOneBooleanContent instead of ZeroOrOneBooleanContent in SIISelLowering.cpp?

The change makes sense to me in general.

When I tried doing the same long time ago, Matt rightly pointed out (https://reviews.llvm.org/D81925#inline-753225) that sign extending does not correspond to the booleans being defined as zero extended. So, wouldn't your change also require an accompanying change of boolean contents to ZeroOrNegativeOneBooleanContent instead of ZeroOrOneBooleanContent in SIISelLowering.cpp?

No, because this is producing a mask and not really an extended value

The change makes sense to me in general.

When I tried doing the same long time ago, Matt rightly pointed out (https://reviews.llvm.org/D81925#inline-753225) that sign extending does not correspond to the booleans being defined as zero extended. So, wouldn't your change also require an accompanying change of boolean contents to ZeroOrNegativeOneBooleanContent instead of ZeroOrOneBooleanContent in SIISelLowering.cpp?

No, because this is producing a mask and not really an extended value

Well in this context it's ambiguous since we don't know what the destination register is used for. I think this is a problem earlier in the flow

The change makes sense to me in general.

When I tried doing the same long time ago, Matt rightly pointed out (https://reviews.llvm.org/D81925#inline-753225) that sign extending does not correspond to the booleans being defined as zero extended. So, wouldn't your change also require an accompanying change of boolean contents to ZeroOrNegativeOneBooleanContent instead of ZeroOrOneBooleanContent in SIISelLowering.cpp?

No, because this is producing a mask and not really an extended value

Well in this context it's ambiguous since we don't know what the destination register is used for. I think this is a problem earlier in the flow

Would you like to talk a little more about the ambiguity in your mind? Sounds like you want the COPY to be dedicated for one of the ambiguous situations.

I think the cause of the issue is that in GlobalISel we are defining the COPY from SCC to SGPR as select 0/1 based on SCC, But in SelectionDAG, the COPY from SCC to SGPR means select 0/-1 based on SCC. And both paths will have some of such copies to be lowered in copyPhysReg(). I have lowered the copy earlier in D109889.

ruiling abandoned this revision.Sep 29 2021, 7:20 PM