AMDGPU: Avoid folding 2 constant operands into an SALU operation

[dstuttard]

Summary

Catch the (admittedly unusual) case where SIFoldOperands attempts to fold 2
constant operands into the same SALU operation, with neither operand able to be
encoded as an inline constant.

Change-Id: Ibc48d662c9ffd8bbacd154976b0b1c257ace0927

[dstuttard]

Formatting

[dstuttard]

I would have preferred to have put this check into isImmOperandLegal in SIInstrInfo.cpp - but that produced lots of lit regressions. Looks like commute operations use this function even when they are swapping rather than replacing the operand (which breaks).

[arsenm]

I've never completely liked the way isOperandLegal is structured to always check the legality with respect to all other operands.

llvm/lib/Target/AMDGPU/SIFoldOperands.cpp

437

TRI is already a class member

440

Demorgan this, and I would order TRI.opCanUseInlineConstant(OpInfo.OperandType) before isInlineConstant

445–446

This isn't the right check for a non-inline constant. It won't catch frame index for example.

llvm/test/CodeGen/AMDGPU/fold-sgpr-multi-imm.mir

13

Is this just hiding not handling every possible case in tryConstantFoldOp? Can you add a test with a more exotic instruction we're unlikely to ever try to constant fold here?

Another case that should always break is a frame index operand

[dstuttard]

Made suggested changes

llvm/lib/Target/AMDGPU/SIFoldOperands.cpp

437

I couldn't work out if you were objecting to the name "TRI" or if you wanted me to re-use TRI from the class object.
I decided to rename it to SRI since to re-use TRI requires a more pervasive change and an extra cast as well.

llvm/test/CodeGen/AMDGPU/fold-sgpr-multi-imm.mir

13

I wasn't sure if the instruction I've chosen is now obscure enough - give me a different suggestion if you can think of something better. (Also added the frameindex case).

[arsenm]

llvm/lib/Target/AMDGPU/SIFoldOperands.cpp

437

I mean re-use. I'm surprised a non-target reference to TRI exists anywhere in this pass

llvm/lib/Target/AMDGPU/SIFoldOperands.cpp

if (!TII->isOperandLegal(*MI, CommuteOpNo, OpToFold)) {
       TII->commuteInstruction(*MI, false, CommuteIdx0, CommuteIdx1);
       return false;
     }
 
     appendFoldCandidate(FoldList, MI, CommuteOpNo, OpToFold, true);
     return true;
   }
 
+  // Check the case where we might introduce a second constant operand to a
+  // scalar instruction
+  if (TII->isSALU(MI->getOpcode())) {
+    const MCInstrDesc &InstDesc = MI->getDesc();
+    const MCOperandInfo &OpInfo = InstDesc.OpInfo[OpNo];
+    const SIRegisterInfo &SRI = TII->getRegisterInfo();
+
+    // Fine if the operand can be encoded as an inline constant
+    if (OpToFold->isImm()) {
+      if (!SRI.opCanUseInlineConstant(OpInfo.OperandType) ||
+          !TII->isInlineConstant(*OpToFold, OpInfo)) {
+        // Otherwise check for another constant
+        for (unsigned i = 0, e = InstDesc.getNumOperands(); i != e; ++i) {
+          auto &Op = MI->getOperand(i);
+          if (OpNo != i &&
+              TII->isLiteralConstantLike(Op, OpInfo)) {
+            return false;
+          }
+        }
+      }
+    }
+  }
+
   appendFoldCandidate(FoldList, MI, OpNo, OpToFold);
   return true;
 }
 
 // If the use operand doesn't care about the value, this may be an operand only
 // used for register indexing, in which case it is unsafe to fold.
 static bool isUseSafeToFold(const SIInstrInfo *TII,
                             const MachineInstr &MI,

llvm/test/CodeGen/AMDGPU/fold-sgpr-multi-imm.mir

+# RUN: llc -march=amdgcn -verify-machineinstrs -run-pass si-fold-operands %s -o - | FileCheck -check-prefix=GCN %s
+
+# GCN-LABEL: name: test_part_fold{{$}}
+# GCN: %2:sreg_32 = S_ADD_I32 70, %1
+---
+name: test_part_fold
+tracksRegLiveness: true
+body: |
+  bb.0:
+    %0:sreg_32 = S_MOV_B32 70
+    %1:sreg_32 = S_MOV_B32 80
+    %2:sreg_32 = S_ADD_I32 %0, %1, implicit-def $scc
+...
+
+# GCN-LABEL: name: test_inline_const{{$}}
+# GCN: %2:sreg_32 = S_ADD_I32 70, 63
+---
+name: test_inline_const
+tracksRegLiveness: true
+body: |
+  bb.0:
+    %0:sreg_32 = S_MOV_B32 70
+    %1:sreg_32 = S_MOV_B32 63
+    %2:sreg_32 = S_ADD_I32 %0, %1, implicit-def $scc
+...
+# GCN-LABEL: name: test_obscure{{$}}
+# GCN: %2:sreg_32 = S_LSHL2_ADD_U32 70, %1
+---
+name: test_obscure
+tracksRegLiveness: true
+body: |
+  bb.0:
+    %0:sreg_32 = S_MOV_B32 70
+    %1:sreg_32 = S_MOV_B32 80
+    %2:sreg_32 = S_LSHL2_ADD_U32 %0, %1, implicit-def $scc
+...
+# GCN-LABEL: name: test_obscure_inline{{$}}
+# GCN: %2:sreg_32 = S_LSHL2_ADD_U32 70, 63
+---
+name: test_obscure_inline
+tracksRegLiveness: true
+body: |
+  bb.0:
+    %0:sreg_32 = S_MOV_B32 70
+    %1:sreg_32 = S_MOV_B32 63
+    %2:sreg_32 = S_LSHL2_ADD_U32 %0, %1, implicit-def $scc
+...
+# GCN-LABEL: name: test_frameindex{{$}}
+# GCN: %1:sreg_32 = S_ADD_I32 %stack.0, %0
+---
+name: test_frameindex
+tracksRegLiveness: true
+stack:
+  - { id: 0, type: default, offset: 0, size: 64, alignment: 16}
+body: |
+  bb.0:
+    %0:sreg_32 = S_MOV_B32 70
+    %1:sreg_32 = S_ADD_I32 %stack.0, %0, implicit-def $scc
+...
+# GCN-LABEL: name: test_frameindex_inline{{$}}
+# GCN: %1:sreg_32 = S_ADD_I32 %stack.0, 63
+---
+name: test_frameindex_inline
+tracksRegLiveness: true
+stack:
+  - { id: 0, type: default, offset: 0, size: 64, alignment: 16}
+body: |
+  bb.0:
+    %0:sreg_32 = S_MOV_B32 63
+    %1:sreg_32 = S_ADD_I32 %stack.0, %0, implicit-def $scc
+...