[PATCH] D54882: [AMDGPU] Add sdwa support for ADD|SUB U64 decomposed Pseudos

[Stanislav Mekhanoshin via Phabricator via llvm-commits]

rampitec added inline comments.


================
Comment at: lib/Target/AMDGPU/SIPeepholeSDWA.cpp:915
+  for (MachineBasicBlock::const_iterator I = MI; I != MISucc;
+       I = std::next(I)) {
+    if (I->modifiesRegister(AMDGPU::VCC, TRI))
----------------
I mean you do not have to check MI itself. ++I was OK:


```
MachineBasicBlock::const_iterator I = std::next(MI);
```


================
Comment at: test/CodeGen/AMDGPU/sdwa-ops.mir:354
+    %63:vgpr_32, %65:sreg_64_xexec = V_ADD_I32_e64 %30.sub0, %23, implicit $exec
+    %31:vreg_64 = COPY $vcc_hi
+    %64:vgpr_32, %66:sreg_64_xexec = V_ADDC_U32_e64 %30.sub1, %0, %65, implicit $exec
----------------
ronlieb wrote:
> ronlieb wrote:
> > rampitec wrote:
> > > I wander why verifier does not complain on this instruction. Ah, I see. Please add -verify-machineinstrs to run lines.
> > > 
> > > Anyway, you need a test for what you are checking in the code: vcc def in between of two instructions.
> > good catch: adding -verify-machineinstrs
> > 
> > see line 364 , this has a def of $vcc between the ADD and ADDC, is that what you are suggesting.
> sorry , i meant line 262
It does not help. You need a test where vcc is defined and killed in between of MI and MISucc.


================
Comment at: test/CodeGen/AMDGPU/sdwa-ops.mir:354
+    %63:vgpr_32, %65:sreg_64_xexec = V_ADD_I32_e64 %30.sub0, %23, implicit $exec
+    %31:vreg_64 = COPY $vcc_hi
+    %64:vgpr_32, %66:sreg_64_xexec = V_ADDC_U32_e64 %30.sub1, %0, %65, implicit $exec
----------------
I still do not see how is it legal to copy 32 bit register into 64 bit.


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

https://reviews.llvm.org/D54882