[PATCH] D94746: [AMDGPU] Move kill lowering to WQM pass and add live mask tracking

[Carl Ritson via Phabricator via llvm-commits]

critson marked 4 inline comments as done.
critson added inline comments.


================
Comment at: llvm/lib/Target/AMDGPU/SIWholeQuadMode.cpp:693
+  case ISD::SETEQ:
+    Opcode = AMDGPU::V_CMP_LG_F32_e64;
+    break;
----------------
foad wrote:
> critson wrote:
> > foad wrote:
> > > I still don't trust this table! I think the "O" predicates should map to "N" opcodes e.g. SETOLT -> V_CMP_NLT as explained above.
> > Are you expecting the table to look like this?
> > ```
> >   case ISD::SETEQ:
> >     Opcode = AMDGPU::V_CMP_LG_F32_e64;
> >     break;
> >   case ISD::SETGT:
> >     Opcode = AMDGPU::V_CMP_GE_F32_e64;
> >     break;
> >   case ISD::SETGE:
> >     Opcode = AMDGPU::V_CMP_GT_F32_e64;
> >     break;
> >   case ISD::SETLT:
> >     Opcode = AMDGPU::V_CMP_LE_F32_e64;
> >     break;
> >   case ISD::SETLE:
> >     Opcode = AMDGPU::V_CMP_LT_F32_e64;
> >     break;
> >   case ISD::SETNE:
> >     Opcode = AMDGPU::V_CMP_EQ_F32_e64;
> >     break;
> >   case ISD::SETO:
> >     Opcode = AMDGPU::V_CMP_O_F32_e64;
> >     break;
> >   case ISD::SETUO:
> >     Opcode = AMDGPU::V_CMP_U_F32_e64;
> >     break;
> >   case ISD::SETOEQ:
> >   case ISD::SETUEQ:
> >     Opcode = AMDGPU::V_CMP_NEQ_F32_e64;
> >     break;
> >   case ISD::SETOGT:
> >   case ISD::SETUGT:
> >     Opcode = AMDGPU::V_CMP_NLT_F32_e64;
> >     break;
> >   case ISD::SETOGE:
> >   case ISD::SETUGE:
> >     Opcode = AMDGPU::V_CMP_NLE_F32_e64;
> >     break;
> >   case ISD::SETOLT:
> >   case ISD::SETULT:
> >     Opcode = AMDGPU::V_CMP_NGT_F32_e64;
> >     break;
> >   case ISD::SETOLE:
> >   case ISD::SETULE:
> >     Opcode = AMDGPU::V_CMP_NGE_F32_e64;
> >     break;
> >   case ISD::SETONE:
> >   case ISD::SETUNE:
> >     Opcode = AMDGPU::V_CMP_NLG_F32_e64;
> >     break;
> > ```
> > 
> > I am still testing, but I am unsure if VulkanCTS or game shaders can tell the differences.
> No I'm expecting it to look like this:
> ```
> case ISD::SETOEQ:
> case ISD::SETEQ:
>   Opcode = AMDGPU::V_CMP_NEQ_F32_e64;
>   break;
> case ISD::SETOGT:
> case ISD::SETGT:
>   Opcode = AMDGPU::V_CMP_NLT_F32_e64;
>   break;
> case ISD::SETOGE:
> case ISD::SETGE:
>   Opcode = AMDGPU::V_CMP_NLE_F32_e64;
>   break;
> case ISD::SETOLT:
> case ISD::SETLT:
>   Opcode = AMDGPU::V_CMP_NGT_F32_e64;
>   break;
> case ISD::SETOLE:
> case ISD::SETLE:
>   Opcode = AMDGPU::V_CMP_NGE_F32_e64;
>   break;
> case ISD::SETONE:
>   Opcode = AMDGPU::V_CMP_NLG_F32_e64;
>   break;
> case ISD::SETO:
>   Opcode = AMDGPU::V_CMP_U_F32_e64;
>   break;
> case ISD::SETUO:
>   Opcode = AMDGPU::V_CMP_O_F32_e64;
>   break;
> case ISD::SETUEQ:
>   Opcode = AMDGPU::V_CMP_LG_F32_e64;
>   break;
> case ISD::SETUGT:
>   Opcode = AMDGPU::V_CMP_GE_F32_e64;
>   break;
> case ISD::SETUGE:
>   Opcode = AMDGPU::V_CMP_GT_F32_e64;
>   break;
> case ISD::SETULT:
>   Opcode = AMDGPU::V_CMP_LE_F32_e64;
>   break;
> case ISD::SETULE:
>   Opcode = AMDGPU::V_CMP_LT_F32_e64;
>   break;
> case ISD::SETUNE:
> case ISD::SETNE:
>   Opcode = AMDGPU::V_CMP_EQ_F32_e64;
>   break;
> ```
> E.g. SETOGT --(swap operands)--> SETOLT --(invert)-->SETUGE which is V_CMP_NLT.
> 
> (Though as mentioned previously, SETEQ is undefined on NaNs, so it doesn't matter whether it behaves the same as SETOEQ or the same as SETUEQ. And likewise for all the others that don't have an explicit O or U.)
Thanks -- I have now validated this version as well.
So I suspect in practice we are not generating anything that hits the edge cases.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D94746/new/

https://reviews.llvm.org/D94746