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

[Ron Lieberman via Phabricator via llvm-commits]

ronlieb marked 2 inline comments as done.
ronlieb 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))
----------------
rampitec wrote:
> I mean you do not have to check MI itself. ++I was OK:
> 
> 
> ```
> MachineBasicBlock::const_iterator I = std::next(MI);
> ```
i could not use std::next(MI) in the initializer, it caused bus errors for consective  MI,MISucc.

This works 
+  // Check if VCC is referenced in range of (MI,MISucc].
+  MachineBasicBlock::const_iterator I = MI;
+  for (++I; I != MISucc; ++I) {



================
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
----------------
rampitec wrote:
> 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.
added subtest test12_add_co_sdwa

# test for $vcc defined and used between adds, should not generate
# GFX9-LABEL: name:            test12_add_co_sdwa



================
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
----------------
rampitec wrote:
> I still do not see how is it legal to copy 32 bit register into 64 bit.
Fixed it at lines 351,354 , and also above at lines 321,325


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

https://reviews.llvm.org/D54882