[PATCH] D87285: AMDGPU/GlobalISelemitter Support for predicate code that uses operands

[Matt Arsenault via Phabricator via llvm-commits]

arsenm added inline comments.


================
Comment at: llvm/include/llvm/CodeGen/GlobalISel/InstructionSelector.h:257
 
+  /// Record operand for predicate code that uses operands.
+  /// - InsnID - Instruction ID
----------------
Can you be clearer on what "uses operands" means here?


================
Comment at: llvm/include/llvm/CodeGen/GlobalISel/InstructionSelector.h:454
     RecordedMIVector MIs;
+    SmallVector<MachineOperand *, 3> RecordedOperands;
     DenseMap<unsigned, unsigned> TempRegisters;
----------------
Is this the recorded use or def operand? Can you add a comment here?


================
Comment at: llvm/lib/CodeGen/GlobalISel/InstructionSelector.cpp:33
+    : Renderers(MaxRenderers), MIs() {
+  RecordedOperands.resize(3);
+}
----------------
Why arbitrarily resize this here?


================
Comment at: llvm/lib/Target/AMDGPU/VOP3Instructions.td:607-610
   // The divergence predicate is irrelevant in GlobalISel, as we have
   // proper register bank checks. We also force all VOP instruction
   // operands to VGPR, so we should not need to check the constant bus
   // restriction.
----------------
Should update this comment


================
Comment at: llvm/lib/Target/AMDGPU/VOP3Instructions.td:612-613
   //
   // FIXME: With unlucky SGPR operands, we could penalize code by
   // blocking folding SGPR->VGPR copies later.
   // FIXME: There's no register bank verifier
----------------
As a follow up, we need to do something about inline constants which will end up blocking this. I was thinking we need to start producing constants with VGPR bank with VALU uses prior to selection


================
Comment at: llvm/test/CodeGen/AMDGPU/GlobalISel/add_shl.ll:2-4
+; RUN: llc < %s -global-isel -mtriple=amdgcn-amd-mesa3d -mcpu=fiji -verify-machineinstrs | FileCheck -check-prefix=VI %s
+; RUN: llc < %s -global-isel -mtriple=amdgcn-amd-mesa3d -mcpu=gfx900 -verify-machineinstrs | FileCheck -check-prefix=GFX9 %s
+; RUN: llc < %s -global-isel -mtriple=amdgcn-amd-mesa3d -mcpu=gfx1010 -verify-machineinstrs | FileCheck -check-prefix=GFX10 %s
----------------
Can you move the < %s to the end


================
Comment at: llvm/utils/TableGen/GlobalISelEmitter.cpp:4203
+      OM.addPredicate<RecordNamedOperandMatcher>(StoreIdxForName[Name], Name);
+      if (!(--WaitingForNamedOperands))
+        StoreIdxForName.clear();
----------------
Can you move the decrement out of the if?


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

https://reviews.llvm.org/D87285