[PATCH] D70896: AMDGPU: Avoid folding 2 constant operands into an SALU operation

[Matt Arsenault via Phabricator via llvm-commits]

arsenm added a comment.

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



================
Comment at: llvm/lib/Target/AMDGPU/SIFoldOperands.cpp:437
+    const MCOperandInfo &OpInfo = InstDesc.OpInfo[OpNo];
+    const SIRegisterInfo &TRI = TII->getRegisterInfo();
+
----------------
TRI is already a class member


================
Comment at: llvm/lib/Target/AMDGPU/SIFoldOperands.cpp:440
+    // Fine if the operand can be encoded as an inline constant
+    if (!(OpToFold->isImm() && TII->isInlineConstant(*OpToFold, OpInfo) &&
+          TRI.opCanUseInlineConstant(OpInfo.OperandType))) {
----------------
Demorgan this, and I would order TRI.opCanUseInlineConstant(OpInfo.OperandType) before isInlineConstant


================
Comment at: llvm/lib/Target/AMDGPU/SIFoldOperands.cpp:445-446
+        auto &Op = MI->getOperand(i);
+        if (OpNo != i && !Op.isReg() && Op.isImm() &&
+            !TII->isInlineConstant(Op, OpInfo)) {
+          return false;
----------------
This isn't the right check for a non-inline constant. It won't catch frame index for example.


================
Comment at: llvm/test/CodeGen/AMDGPU/fold-sgpr-multi-imm.mir:12
+    %1:sreg_32 = S_MOV_B32 80
+    %2:sreg_32 = S_ADD_I32 %0, %1, implicit-def $scc
+...
----------------
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


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D70896