[AMDGPU] Remove unnecessary movs for v_fmac operands

[rtaylor]

Summary

This propagates a sgpr->vgpr copy into an operand of a
v_fma/v_mad that is generated from v_fmac, given it
doesn't break constant bus restriction

Change-Id: I39cacf43205e15982f7405c805d67b7a8b11d2a9

[rtaylor]

Added test case

[arsenm]

I somewhat expected this to be handled in SIFoldOperands, as constants are already folded there and this is essentially the same problem. It will always save an instruction. This is the version RA uses and I'm not sure I expect it to do any folding

lib/Target/AMDGPU/SIInstrInfo.cpp

2630–2633 ↗

(On Diff #216890)

These are the exact conditions as checked above

2636–2637 ↗

(On Diff #216890)

This can fail

2638 ↗

(On Diff #216890)

Extra parentheses

test/CodeGen/AMDGPU/fmac-fma-sgpr-copy.ll

5–13

You should be able to reduce this. You shouldn't need any vector operations

[rtaylor]

Made some changes to code.

[rtaylor]

lib/Target/AMDGPU/SIInstrInfo.cpp

2630–2633 ↗

(On Diff #216890)

Yes, it is. I could create a local function that does this and replace both with that, it would be just as ugly since there are so many conditions (params) to pass, or I could pass MI and re-get all those operands, which would be the exact code that is already in the function.

test/CodeGen/AMDGPU/fmac-fma-sgpr-copy.ll

5–13

I tried a few different things but I probably just couldn't find the simple form to produce a convertable V_FMAC. Suggestions?

[arsenm]

lib/Target/AMDGPU/SIInstrInfo.cpp

2630–2633 ↗

(On Diff #216890)

Why can't you just set a bool flag inside the first condition?

Anyway, I would prefer if the wasn't done here at all, and SIFoldOperands took care of this

[rtaylor]

lib/Target/AMDGPU/SIInstrInfo.cpp

2630–2633 ↗

(On Diff #216890)

Sure, no problem, if you think that looks better.

This test case doesn't seem to be handled by SIFoldOperands (V_FMAC before and V_FMAC after) SIFoldOperands, so doing this change there wouldn't do anything.

test/CodeGen/AMDGPU/fmac-fma-sgpr-copy.ll

5–13

Just to be more explicit, this test case was stripped directly from a shader. If I do something like:

define amdgpu_cs float @test1(<4 x i32> inreg %a, float %b, float %y, float %c) {
entry:

%.i098 = fsub nnan arcp float %b, %c
%fma1 = call float @llvm.fma.f32(float %y, float %.i098, float %b) #3
ret float %fma1

}

results in no v_mov, either inreg for b or not. If it's not inreg than it's a vgpr directly and if it's inreg than it's a sgpr already. Is there a simpler way to take in a vector and convert to scalar?

[rtaylor]

Set bool in condition.

[rtaylor]

Forgot to capitalize var name.

[nhaehnle]

lib/Target/AMDGPU/SIInstrInfo.cpp

2630–2633 ↗

(On Diff #216890)

ConstantBus really shouldn't be assigned inside the if condition itself. I assume @arsenm meant to set that variable in the body.

That said, I also agree that this should happen in SIFoldOperands, and I don't follow your argument here. SIFoldOperands can already do this, e.g. consider

define amdgpu_cs float @test2(float inreg %a, float %b, float %y, float %z) {
entry:
  %fma1 = call float @llvm.fma.f32(float %y, float %z, float %a) #3
  ret float %fma1
}

The FMAC becomes an FMA in SIFoldOperands for this example. It's possible that the reason why it doesn't work in your case is that that code doesn't handle sub-registers.

[rtaylor]

lib/Target/AMDGPU/SIInstrInfo.cpp

2630–2633 ↗

(On Diff #216890)

I took the "inside the first condition" literally.

Yes, you are correct, it seems it's because of subregs use in this test case.

[rtaylor]

lib/Target/AMDGPU/SIInstrInfo.cpp

2630–2633 ↗

(On Diff #216890)

Looks like this was done on purpose, not folding a sub register to a fully defined tied register.

[rtaylor]

Added condition in foldOperands to allow tied subreg folding
for V_FMAC and V_MAC operations

[rtaylor]

Ping.

[arsenm]

Can you also add a MIR test for this? Also testing the 16-bit case would be nice

lib/Target/AMDGPU/SIFoldOperands.cpp

512–514

Can you fold this opcode list into a canFoldTiedSrcOp (or something similar) predicate function

[rtaylor]

https://reviews.llvm.org/D69287 covers this.

lib/Target/AMDGPU/SIFoldOperands.cpp

void SIFoldOperands::foldOperand(
   if (!isUseSafeToFold(TII, *UseMI, UseOp))
     return;
 
   // FIXME: Fold operands with subregs.
   if (UseOp.isReg() && OpToFold.isReg()) {
     if (UseOp.isImplicit() || UseOp.getSubReg() != AMDGPU::NoSubRegister)
       return;
 
-    // Don't fold subregister extracts into tied operands, only if it is a full
-    // copy since a subregister use tied to a full register def doesn't really
-    // make sense. e.g. don't fold:
-    //
-    // %1 = COPY %0:sub1
-    // %2<tied3> = V_MAC_{F16, F32} %3, %4, %1<tied0>
-    //
-    //  into
-    // %2<tied3> = V_MAC_{F16, F32} %3, %4, %0:sub1<tied0>
-    if (UseOp.isTied() && OpToFold.getSubReg() != AMDGPU::NoSubRegister)
+    // Allow folding subregister extract into tied operands for
+    // v_mac and v_fmac opcodes only
+    if (UseOp.isTied() && OpToFold.getSubReg() != AMDGPU::NoSubRegister) {
+      unsigned UseOpc = UseOp.getParent()->getOpcode();
+      if (UseOpc != AMDGPU::V_MAC_F32_e64 &&
+          UseOpc != AMDGPU::V_MAC_F16_e64 &&
+          UseOpc != AMDGPU::V_FMAC_F32_e64)
         return;
     }
+  }
 
   // Special case for REG_SEQUENCE: We can't fold literals into
   // REG_SEQUENCE instructions, so we have to fold them into the
   // uses of REG_SEQUENCE.
   if (UseMI->isRegSequence()) {
     Register RegSeqDstReg = UseMI->getOperand(0).getReg();
     unsigned RegSeqDstSubReg = UseMI->getOperand(UseOpIdx + 1).getImm();
 

test/CodeGen/AMDGPU/fmac-fma-sgpr-copy.ll

+; RUN: llc < %s -march=amdgcn -mcpu=gfx1010 -verify-machineinstrs | FileCheck %s
+
+; CHECK: v_fma_f32 v0, v1, v0, s0
+define amdgpu_cs float @test1(<4 x i32> inreg %a, float %b, float %y) {
+entry:
+  %buf.load = call <4 x i32> @llvm.amdgcn.s.buffer.load.v4i32(<4 x i32> %a, i32 0, i32 0)
+  %vec1 = bitcast <4 x i32> %buf.load to <4 x float>
+  %.i095 = extractelement <4 x float> %vec1, i32 0
+  %.i098 = fsub nnan arcp float %b, %.i095
+  %fma1 = call float @llvm.fma.f32(float %y, float %.i098, float %.i095) #3
+  ret float %fma1
+}
+
+declare <4 x i32> @llvm.amdgcn.s.buffer.load.v4i32(<4 x i32>, i32, i32 immarg) #2
+declare float @llvm.fma.f32(float, float, float) #1
+
+attributes #1 = { nounwind readnone speculatable willreturn }
+attributes #2 = { nounwind readnone }
+attributes #3 = { nounwind }

test/CodeGen/AMDGPU/fmul-2-combine-multi-use.ll

define amdgpu_kernel void @multiple_use_fadd_fmac_f32(float addrspace(1)* %out, float %x, [8 x i32], float %y) #0 {
   store volatile float %mul2, float addrspace(1)* %out
   store volatile float %mad, float addrspace(1)* %out.gep.1
   ret void
 }
 
 ; GCN-LABEL: {{^}}multiple_use_fadd_fmad_f32:
 ; GCN-DAG:   v_add_f32_e64 [[MUL2:v[0-9]+]], |[[X:s[0-9]+]]|, |s{{[0-9]+}}|
 ; SIVI-DAG:  v_mad_f32 [[MAD:v[0-9]+]], |[[X]]|, 2.0, v{{[0-9]+}}
-; GFX10-DAG: v_fma_f32 [[MAD:v[0-9]+]], 2.0, |[[X]]|, v{{[0-9]+}}
+; GFX10-DAG: v_fma_f32 [[MAD:v[0-9]+]], |[[X]]|, 2.0, s{{[0-9]+}}
 ; GCN-DAG:   buffer_store_dword [[MUL2]]
 ; GCN-DAG:   buffer_store_dword [[MAD]]
 ; GCN:       s_endpgm
 define amdgpu_kernel void @multiple_use_fadd_fmad_f32(float addrspace(1)* %out, float %x, float %y) #0 {
   %out.gep.1 = getelementptr float, float addrspace(1)* %out, i32 1
   %x.abs = call float @llvm.fabs.f32(float %x)
   %mul2 = fmul fast float %x.abs, 2.0
   %mad = fadd fast float %mul2, %y