AMDGPU: Partially fix machine uniformity for inline asm

[arsenm]

Summary

This was assuming virtual registers only, and asserting on physical.
This was also ignoring AGPRs, and only considering VGPRs.

Reporting the instruction as uniform or not is conceptually wrong,
this should be reported per-operand. An inline asm statement could
include uniform and non-uniform components. This should report
purely for the register defs and ignore the uses.

Fixes asserting on most of the inline asm tests when uniformity
analysis is used.

[foad]

Is there a way to test this?

[Pierre-vh]

Can this be tested or is it tested as part of D142021?
The changes make sense to me but without seeing codegen changes, it's difficult to really understand what's going on

[arsenm]

In D142020#4064792, @foad wrote:

Is there a way to test this?

The existing tests break if you flip uniformity analysis on, so I think this is mostly well covered. We might want additional asm testing when the API is fixed to be per output

[Pierre-vh]

No more comments from me but I don't have enough experience with uniformity analysis to approve

llvm/lib/Target/AMDGPU/SIInstrInfo.cpp

8407–8416

Variable names need to start with an upper case letter

[arsenm]

llvm/lib/Target/AMDGPU/SIInstrInfo.cpp

8407–8416

This is just moving existing code, rename should be separate

[arsenm]

490e348e67945a4a7e118709579f17b2c96b40bd

llvm/include/llvm/CodeGen/RegisterBankInfo.h

protected:
   /// Get the register bank identified by \p ID.
   RegisterBank &getRegBank(unsigned ID) {
     assert(ID < getNumRegBanks() && "Accessing an unknown register bank");
     return *RegBanks[ID];
   }
 
   /// Get the MinimalPhysRegClass for Reg.
   /// \pre Reg is a physical register.
-  const TargetRegisterClass &
+  const TargetRegisterClass *
   getMinimalPhysRegClass(Register Reg, const TargetRegisterInfo &TRI) const;
 
   /// Try to get the mapping of \p MI.
   /// See getInstrMapping for more details on what a mapping represents.
   ///
   /// Unlike getInstrMapping the returned InstructionMapping may be invalid
   /// (isValid() == false).
   /// This means that the target independent code is not smart enough

llvm/lib/CodeGen/RegisterBankInfo.cpp

 #endif // NDEBUG
   return true;
 }
 
 const RegisterBank *
 RegisterBankInfo::getRegBank(Register Reg, const MachineRegisterInfo &MRI,
                              const TargetRegisterInfo &TRI) const {
   if (Reg.isPhysical()) {
+    assert(Reg && "NoRegister does not have a register bank");
+
     // FIXME: This was probably a copy to a virtual register that does have a
     // type we could use.
-    return &getRegBankFromRegClass(getMinimalPhysRegClass(Reg, TRI), LLT());
+    const TargetRegisterClass *RC = getMinimalPhysRegClass(Reg, TRI);
+    return RC ? &getRegBankFromRegClass(*RC, LLT()) : nullptr;
   }
 
-  assert(Reg && "NoRegister does not have a register bank");
   const RegClassOrRegBank &RegClassOrBank = MRI.getRegClassOrRegBank(Reg);
   if (auto *RB = RegClassOrBank.dyn_cast<const RegisterBank *>())
     return RB;
   if (auto *RC = RegClassOrBank.dyn_cast<const TargetRegisterClass *>())
     return &getRegBankFromRegClass(*RC, MRI.getType(Reg));
   return nullptr;
 }
 
-const TargetRegisterClass &
+const TargetRegisterClass *
 RegisterBankInfo::getMinimalPhysRegClass(Register Reg,
                                          const TargetRegisterInfo &TRI) const {
   assert(Reg.isPhysical() && "Reg must be a physreg");
   const auto &RegRCIt = PhysRegMinimalRCs.find(Reg);
   if (RegRCIt != PhysRegMinimalRCs.end())
-    return *RegRCIt->second;
-  const TargetRegisterClass *PhysRC = TRI.getMinimalPhysRegClass(Reg);
+    return RegRCIt->second;
+  const TargetRegisterClass *PhysRC = TRI.getMinimalPhysRegClassLLT(Reg, LLT());
   PhysRegMinimalRCs[Reg] = PhysRC;
-  return *PhysRC;
+  return PhysRC;
 }
 
 const RegisterBank *RegisterBankInfo::getRegBankFromConstraints(
     const MachineInstr &MI, unsigned OpIdx, const TargetInstrInfo &TII,
     const MachineRegisterInfo &MRI) const {
   const TargetRegisterInfo *TRI = MRI.getTargetRegisterInfo();
 
   // The mapping of the registers may be available via the

 unsigned RegisterBankInfo::getSizeInBits(Register Reg,
                                          const MachineRegisterInfo &MRI,
                                          const TargetRegisterInfo &TRI) const {
   if (Reg.isPhysical()) {
     // The size is not directly available for physical registers.
     // Instead, we need to access a register class that contains Reg and
     // get the size of that register class.
     // Because this is expensive, we'll cache the register class by calling
-    auto *RC = &getMinimalPhysRegClass(Reg, TRI);
+    auto *RC = getMinimalPhysRegClass(Reg, TRI);
     assert(RC && "Expecting Register class");
     return TRI.getRegSizeInBits(*RC);
   }
   return TRI.getRegSizeInBits(Reg, MRI);
 }
 
 //------------------------------------------------------------------------------
 // Helper classes implementation.

llvm/lib/Target/AMDGPU/GCNSubtarget.h

 /// AMD GCN specific subclass of TargetSubtarget.
 //
 //===----------------------------------------------------------------------===//
 
 #ifndef LLVM_LIB_TARGET_AMDGPU_GCNSUBTARGET_H
 #define LLVM_LIB_TARGET_AMDGPU_GCNSUBTARGET_H
 
 #include "AMDGPUCallLowering.h"
+#include "AMDGPURegisterBankInfo.h"
 #include "AMDGPUSubtarget.h"
 #include "SIFrameLowering.h"
 #include "SIISelLowering.h"
 #include "SIInstrInfo.h"
 #include "llvm/CodeGen/SelectionDAGTargetInfo.h"
 
 #define GET_SUBTARGETINFO_HEADER
 #include "AMDGPUGenSubtargetInfo.inc"
public:
   };
 
 private:
   /// GlobalISel related APIs.
   std::unique_ptr<AMDGPUCallLowering> CallLoweringInfo;
   std::unique_ptr<InlineAsmLowering> InlineAsmLoweringInfo;
   std::unique_ptr<InstructionSelector> InstSelector;
   std::unique_ptr<LegalizerInfo> Legalizer;
-  std::unique_ptr<RegisterBankInfo> RegBankInfo;
+  std::unique_ptr<AMDGPURegisterBankInfo> RegBankInfo;
 
 protected:
   // Basic subtarget description.
   Triple TargetTriple;
   AMDGPU::IsaInfo::AMDGPUTargetID TargetID;
   unsigned Gen = INVALID;
   InstrItineraryData InstrItins;
   int LDSBankCount = 0;
public:
   InstructionSelector *getInstructionSelector() const override {
     return InstSelector.get();
   }
 
   const LegalizerInfo *getLegalizerInfo() const override {
     return Legalizer.get();
   }
 
-  const RegisterBankInfo *getRegBankInfo() const override {
+  const AMDGPURegisterBankInfo *getRegBankInfo() const override {
     return RegBankInfo.get();
   }
 
   const AMDGPU::IsaInfo::AMDGPUTargetID &getTargetID() const {
     return TargetID;
   }
 
   // Nothing implemented, just prevent crashes on use.

llvm/lib/Target/AMDGPU/SIInstrInfo.cpp

if (SIInstrInfo::isGenericAtomicRMWOpcode(opcode) ||
       opcode == AMDGPU::G_ATOMIC_CMPXCHG_WITH_SUCCESS) {
     return InstructionUniformity::NeverUniform;
   }
   return InstructionUniformity::Default;
 }
 
 InstructionUniformity
 SIInstrInfo::getInstructionUniformity(const MachineInstr &MI) const {
+  unsigned opcode = MI.getOpcode();
+  if (MI.isCopy()) {
+    const MachineOperand &srcOp = MI.getOperand(1);
+    if (srcOp.isReg() && srcOp.getReg().isPhysical()) {
+      const TargetRegisterClass *regClass =
+          RI.getPhysRegBaseClass(srcOp.getReg());
+      return RI.isSGPRClass(regClass) ? InstructionUniformity::AlwaysUniform
+                                      : InstructionUniformity::NeverUniform;
+    }
+    return InstructionUniformity::Default;
+  }
+
+  // GMIR handling
+  if (MI.isPreISelOpcode())
+    return SIInstrInfo::getGenericInstructionUniformity(MI);
+
   // Atomics are divergent because they are executed sequentially: when an
   // atomic operation refers to the same address in each thread, then each
   // thread after the first sees the value written by the previous thread as
   // original value.
 
   if (isAtomic(MI))
     return InstructionUniformity::NeverUniform;
 
if (llvm::any_of(MI.memoperands(), [](const MachineMemOperand *mmo) {
         })) {
       // At least one MMO in a non-global address space.
       return InstructionUniformity::NeverUniform;
     }
 
     return InstructionUniformity::Default;
   }
 
-  unsigned opcode = MI.getOpcode();
-  if (opcode == AMDGPU::COPY) {
-    const MachineOperand &srcOp = MI.getOperand(1);
-    if (srcOp.isReg() && srcOp.getReg().isPhysical()) {
-      const TargetRegisterClass *regClass = RI.getPhysRegBaseClass(srcOp.getReg());
-      return RI.isSGPRClass(regClass) ? InstructionUniformity::AlwaysUniform
-                                      : InstructionUniformity::NeverUniform;
-    }
-    return InstructionUniformity::Default;
-  }
-  if (opcode == AMDGPU::INLINEASM || opcode == AMDGPU::INLINEASM_BR) {
-    const MachineRegisterInfo &MRI = MI.getParent()->getParent()->getRegInfo();
-    for (auto &op : MI.operands()) {
-      if (!op.isReg() || !op.isDef())
-        continue;
-      auto *RC = MRI.getRegClass(op.getReg());
-      if (!RC || RI.isDivergentRegClass(RC))
-        return InstructionUniformity::NeverUniform;
-    }
-    return InstructionUniformity::AlwaysUniform;
-  }
   if (opcode == AMDGPU::V_READLANE_B32 || opcode == AMDGPU::V_READFIRSTLANE_B32)
     return InstructionUniformity::AlwaysUniform;
 
   if (opcode == AMDGPU::V_WRITELANE_B32)
     return InstructionUniformity::NeverUniform;
 
-  // GMIR handling
-  if (SIInstrInfo::isGenericOpcode(opcode))
-    return SIInstrInfo::getGenericInstructionUniformity(MI);
+  const MachineRegisterInfo &MRI = MI.getParent()->getParent()->getRegInfo();
+  const AMDGPURegisterBankInfo *RBI = ST.getRegBankInfo();
 
-  // Handling $vpgr reads
-  for (auto srcOp : MI.operands()) {
-    if (srcOp.isReg() && srcOp.getReg().isPhysical()) {
-      const TargetRegisterClass *regClass = RI.getPhysRegBaseClass(srcOp.getReg());
+  // FIXME: It's conceptually broken to report this for an instruction, and not
+  // a specific def operand. For inline asm in particular, there could be mixed
+  // uniform and divergent results.
+  for (unsigned I = 0, E = MI.getNumOperands(); I != E; ++I) {
+    const MachineOperand &SrcOp = MI.getOperand(I);
+    if (!SrcOp.isReg())
+      continue;
 
-      // If the class is missing it's an unallocatable scalar of some kind.
-      if (!regClass)
-        continue;
+    Register Reg = SrcOp.getReg();
+    if (!Reg || !SrcOp.readsReg())
 
-      if (RI.isVGPRClass(regClass))
-        return InstructionUniformity::NeverUniform;
-    }
-  }
+    // If RegBank is null, this is unassigned or an unallocatable special
+    // register, which are all scalars.
+    const RegisterBank *RegBank = RBI->getRegBank(Reg, MRI, RI);
+    if (RegBank && RegBank->getID() != AMDGPU::SGPRRegBankID)
 
   // TODO: Uniformity check condtions above can be rearranged for more
   // redability
 
   // TODO: amdgcn.{ballot, [if]cmp} should be AlwaysUniform, but they are
   //       currently turned into no-op COPYs by SelectionDAG ISel and are
   //       therefore no longer recognizable.