[PATCH] D40851: [AMDGPU] Improve verifier wrt vcc subregs

[Matt Arsenault via Phabricator via llvm-commits]

arsenm added inline comments.


================
Comment at: lib/Target/AMDGPU/SIInstrInfo.cpp:2458
   // SGPRs use the constant bus
-  return (MO.getReg() == AMDGPU::VCC || MO.getReg() == AMDGPU::M0 ||
+  return (MO.getReg() == AMDGPU::VCC || MO.getReg() == AMDGPU::VCC_LO ||
+          MO.getReg() == AMDGPU::VCC_HI || MO.getReg() == AMDGPU::M0 ||
----------------
I was trying to clean this up recently to avoid having to list VCC here at all by checking SReg_32/SReg_64 classes instead. I think there's an issue here because implicit really means two different things. This really wants to be checking for the hardware implicit physical register uses from the instruction definition, which only applies to VCC. No instructions implicitly read just vcc_lo or vcc_hi. Implicit can also mean extra operands the compiler added on for modeling other constraints, which should not be verified for constant bus usage.




================
Comment at: lib/Target/AMDGPU/SIInstrInfo.cpp:2473-2474
     case AMDGPU::VCC:
+    case AMDGPU::VCC_LO:
+    case AMDGPU::VCC_HI:
     case AMDGPU::M0:
----------------
There aren't any instructions that use these for implicit operands.

In the first test you mentioned, I would hope this would be skipping any of the extra implicit operands beyond the defined exec use since they aren't actually going to be used by the hardware and are just for the compiler internals.


================
Comment at: lib/Target/AMDGPU/SIInstrInfo.cpp:2739
         if (MO.isReg()) {
-          if (MO.getReg() != SGPRUsed)
+          if (SGPRUsed == AMDGPU::NoRegister ||
+              !RI.regsOverlap(MO.getReg(), SGPRUsed))
----------------
Why would noreg count as a constant bus use? I wouldn't expect to see a noreg operand ever reach here


================
Comment at: lib/Target/AMDGPU/SIInstrInfo.cpp:2740
+          if (SGPRUsed == AMDGPU::NoRegister ||
+              !RI.regsOverlap(MO.getReg(), SGPRUsed))
             ++ConstantBusCount;
----------------
I'm not sure this is correct. Consider:

v_cndmask_b32_e32 v0, vcc_hi, v1, <implicit vcc>

I'm not sure if this is valid or not.


https://reviews.llvm.org/D40851