[PATCH] D141127: AMDGPU/GlobalISel: Widen s1 SGPR constants during regbankselect

[Pierre van Houtryve via Phabricator via llvm-commits]

Pierre-vh added inline comments.


================
Comment at: llvm/lib/Target/AMDGPU/AMDGPURegisterBankInfo.cpp:2143
+        ConstantInt::get(IntegerType::getInt32Ty(Ctx),
+                         MI.getOperand(1).getCImm()->getZExtValue()));
+    MRI.setRegBank(NewDstReg, *DstBank);
----------------
Small nit: Can we put this on a separate line to reduce nesting? e.g.
`const auto Val = MI.getOperand(1).getCImm()->getZExtValue()`


================
Comment at: llvm/test/CodeGen/AMDGPU/GlobalISel/bool-legalization.ll:71
 ; WAVE64-NEXT:    s_waitcnt lgkmcnt(0)
-; WAVE64-NEXT:    s_xor_b32 s0, s0, -1
+; WAVE64-NEXT:    s_xor_b32 s0, s0, 1
 ; WAVE64-NEXT:    s_and_b32 s0, s0, 1
----------------
If I understand correctly, it was interpreted as 1 on all lanes before (so -1 as a 32 bit value) but now we're widening the constant so it's just 1, and the end result is the same?


================
Comment at: llvm/test/CodeGen/AMDGPU/GlobalISel/br-constant-invalid-sgpr-copy.ll:5
+
+; This was mishandling the constant true and false values used as a
+; scalar branch condition.
----------------
It would have been nice to have the test pre-committed to see exactly what has been fixed, but it's a small test/diff so I'm not going to ask to split it
Can you paste the pre-change ISA in a comment so I can see the before/after?


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

https://reviews.llvm.org/D141127