[AMDGPU] Inline asm: use AsmNames for register constraints.

[arsenm]

lib/Target/AMDGPU/SIRegisterInfo.cpp

15

Should not be first include.

Why is SIRegisterInfo including from the InstPrinter? This seems like a layering violation

[SamWot]

lib/Target/AMDGPU/SIRegisterInfo.cpp

15

SIRegisterInfo needs access to AMDGPUInstPrinter::getRegisterName() to obtain AsmNames for registers. This is the only way to get it currently without changing core LLVM.
See this review: http://reviews.llvm.org/D19010

[nhaustov]

Looks good.

inline-reg-constraints.ll -> inline-asm-reg-constraints.ll

[• tstellarAMD]

Matt's concern about there being a layer violation is valid. We need to resolve this before the patch can be committed.

[SamWot]

Is layering violation a strong prohibition for acceptance?
There is formal layering violation that RegisterInfo calls InstPrinter but for me it looks like it is conceptual layering violatoin in LLVM architecture. Assembly is responsibility of MC Layer but inline assembly is responsibility of TargetLowering and RegisterInfo (TargetLowering::getRegForInlineAsmConstraint() and RegisterInfo::getRegAsmName()).
So without code dupplication or major changes in core LLVM I don't see how this violation can be solved.
Possibly we should get used to that we must use names like VGPR13 or rename our Register classes.

[arsenm]

It seems strange to me that this is here, instead of someplace like MCAsmInfo. Why was getRegAsmName added to TRI?

[SamWot]

getRegAsmName() is used by TargetLowering::getRegForInlineAsmConstraint(). Placing it in MCAsmInfo will introduce layering violation in the same way.

[SamWot]

What should I do with this change?
I don't see ways to solve layering violation without changes in core LLVM.

[• tstellarAMD]

In D19116#410580, @SamWot wrote:

What should I do with this change?
I don't see ways to solve layering violation without changes in core LLVM.

The easiest way to avoid a potential layering violation would be to have TableGen emit AMDGPUInstPrinter::getRegisterName(Reg) as static function rather than an AMDGPUInstPrinter member function.

However, if including the InstPrinter headers from the CodeGen files is a layering violation, all targets are already doing this by including InstrPrinter headers in XXXAsmPrinter. So, this change really isn't doing anything that isn't already being done. I'm fine with this being committed.

[Eugene.Zelenko]

Looks like patch was not committed.

lib/Target/AMDGPU/SIRegisterInfo.h

public:
   bool requiresRegisterScavenging(const MachineFunction &Fn) const override;
 
   bool requiresFrameIndexScavenging(const MachineFunction &MF) const override;
 
   void eliminateFrameIndex(MachineBasicBlock::iterator MI, int SPAdj,
                            unsigned FIOperandNum,
                            RegScavenger *RS) const override;
 
+  std::string getRegAsmName(unsigned Reg) const override;
+
   unsigned getHWRegIndex(unsigned Reg) const override;
 
   /// \brief Return the 'base' register class for this register.
   /// e.g. SGPR0 => SReg_32, VGPR => VGPR_32 SGPR0_SGPR1 -> SReg_32, etc.
   const TargetRegisterClass *getPhysRegClass(unsigned Reg) const;
 
   /// \returns true if this class contains only SGPR registers
   bool isSGPRClass(const TargetRegisterClass *RC) const {

lib/Target/AMDGPU/SIRegisterInfo.cpp

 //===-- SIRegisterInfo.cpp - SI Register Information ---------------------===//
 //
 //                     The LLVM Compiler Infrastructure
 //
 // This file is distributed under the University of Illinois Open Source
 // License. See LICENSE.TXT for details.
 //
 //===----------------------------------------------------------------------===//
 //
 /// \file
 /// \brief SI implementation of the TargetRegisterInfo class.
 //
 //===----------------------------------------------------------------------===//
 
 #include "SIRegisterInfo.h"
+#include "InstPrinter/AMDGPUInstPrinter.h"
 #include "SIInstrInfo.h"
 #include "SIMachineFunctionInfo.h"
 #include "llvm/CodeGen/MachineFrameInfo.h"
 #include "llvm/CodeGen/MachineInstrBuilder.h"
 #include "llvm/CodeGen/RegisterScavenging.h"
 #include "llvm/IR/Function.h"
 #include "llvm/IR/LLVMContext.h"
 
default: {
                 TII->get(AMDGPU::V_MOV_B32_e32), TmpReg)
                 .addImm(Offset);
         FIOp.ChangeToRegister(TmpReg, false, false, true);
       }
     }
   }
 }
 
+std::string SIRegisterInfo::getRegAsmName(unsigned Reg) const {
+  return StringRef(AMDGPUInstPrinter::getRegisterName(Reg));
+}
+
+
 unsigned SIRegisterInfo::getHWRegIndex(unsigned Reg) const {
   return getEncodingValue(Reg) & 0xff;
 }
 
 // FIXME: This is very slow. It might be worth creating a map from physreg to
 // register class.
 const TargetRegisterClass *SIRegisterInfo::getPhysRegClass(unsigned Reg) const {
   assert(!TargetRegisterInfo::isVirtualRegister(Reg));

lib/Target/AMDGPU/SIRegisterInfo.td

 def TTMP8 : SIReg <"ttmp8", 120>;
 def TTMP9 : SIReg <"ttmp9", 121>;
 def TTMP10 : SIReg <"ttmp10", 122>;
 def TTMP11 : SIReg <"ttmp11", 123>;
 
 multiclass FLAT_SCR_LOHI_m <string n, bits<16> ci_e, bits<16> vi_e> {
   def _ci : SIReg<n, ci_e>;
   def _vi : SIReg<n, vi_e>;
-  def "" : SIReg<"", 0>;
+  def "" : SIReg<n, 0>;
 }
 
 class FlatReg <Register lo, Register hi, bits<16> encoding> :
     RegisterWithSubRegs<"flat_scratch", [lo, hi]>,
     DwarfRegAlias<lo> {
   let Namespace = "AMDGPU";
   let SubRegIndices = [sub0, sub1];
   let HWEncoding = encoding;
 }
 
 defm FLAT_SCR_LO : FLAT_SCR_LOHI_m<"flat_scratch_lo", 104, 102>; // Offset in units of 256-bytes.
 defm FLAT_SCR_HI : FLAT_SCR_LOHI_m<"flat_scratch_hi", 105, 103>; // Size is the per-thread scratch size, in bytes.
 
 def FLAT_SCR_ci : FlatReg<FLAT_SCR_LO_ci, FLAT_SCR_HI_ci, 104>;
 def FLAT_SCR_vi : FlatReg<FLAT_SCR_LO_vi, FLAT_SCR_HI_vi, 102>;
 def FLAT_SCR : FlatReg<FLAT_SCR_LO, FLAT_SCR_HI, 0>;
 
 // SGPR registers
 foreach Index = 0-103 in {
-  def SGPR#Index : SIReg <"SGPR"#Index, Index>;
+  def SGPR#Index : SIReg <"s"#Index, Index>;
 }
 
 // VGPR registers
 foreach Index = 0-255 in {
-  def VGPR#Index : SIReg <"VGPR"#Index, Index> {
+  def VGPR#Index : SIReg <"v"#Index, Index> {
     let HWEncoding{8} = 1;
   }
 }
 
 //===----------------------------------------------------------------------===//
 //  Groupings using register classes and tuples
 //===----------------------------------------------------------------------===//
 

test/CodeGen/AMDGPU/flat-scratch-reg.ll

 ; RUN: llc < %s -march=amdgcn -mcpu=kaveri -verify-machineinstrs | FileCheck %s --check-prefix=GCN --check-prefix=CI --check-prefix=NO-XNACK
 ; RUN: llc < %s -march=amdgcn -mcpu=fiji -verify-machineinstrs | FileCheck %s --check-prefix=GCN --check-prefix=VI --check-prefix=NO-XNACK
 ; RUN: llc < %s -march=amdgcn -mcpu=carrizo -mattr=+xnack -verify-machineinstrs | FileCheck %s --check-prefix=GCN --check-prefix=VI --check-prefix=XNACK
 
 ; GCN-LABEL: {{^}}no_vcc_no_flat:
 ; NO-XNACK: ; NumSgprs: 8
 ; XNACK: ; NumSgprs: 12
 define void @no_vcc_no_flat() {
 entry:
-  call void asm sideeffect "", "~{SGPR7}"()
+  call void asm sideeffect "", "~{s7}"()
   ret void
 }
 
 ; GCN-LABEL: {{^}}vcc_no_flat:
 ; NO-XNACK: ; NumSgprs: 10
 ; XNACK: ; NumSgprs: 12
 define void @vcc_no_flat() {
 entry:
-  call void asm sideeffect "", "~{SGPR7},~{VCC}"()
+  call void asm sideeffect "", "~{s7},~{vcc}"()
   ret void
 }
 
 ; GCN-LABEL: {{^}}no_vcc_flat:
 ; CI: ; NumSgprs: 12
 ; VI: ; NumSgprs: 14
 define void @no_vcc_flat() {
 entry:
-  call void asm sideeffect "", "~{SGPR7},~{FLAT_SCR}"()
+  call void asm sideeffect "", "~{s7},~{flat_scratch}"()
   ret void
 }
 
 ; GCN-LABEL: {{^}}vcc_flat:
 ; CI: ; NumSgprs: 12
 ; VI: ; NumSgprs: 14
 define void @vcc_flat() {
 entry:
-  call void asm sideeffect "", "~{SGPR7},~{VCC},~{FLAT_SCR}"()
+  call void asm sideeffect "", "~{s7},~{vcc},~{flat_scratch}"()
   ret void
 }

test/CodeGen/AMDGPU/inline-reg-constraints.ll

+; RUN: llc < %s -march=amdgcn -mcpu=kaveri -verify-machineinstrs | FileCheck %s --check-prefix=GCN --check-prefix=CI --check-prefix=NO-XNACK
+; RUN: llc < %s -march=amdgcn -mcpu=fiji -verify-machineinstrs | FileCheck %s --check-prefix=GCN --check-prefix=VI --check-prefix=NO-XNACK
+; RUN: llc < %s -march=amdgcn -mcpu=kaveri -verify-machineinstrs | FileCheck %s --check-prefix=GCN --check-prefix=CI --check-prefix=NO-XNACK-CI
+; RUN: llc < %s -march=amdgcn -mcpu=fiji -verify-machineinstrs | FileCheck %s --check-prefix=GCN --check-prefix=VI --check-prefix=NO-XNACK-VI
+; RUN: llc < %s -march=amdgcn -mcpu=carrizo -mattr=+xnack -verify-machineinstrs | FileCheck %s --check-prefix=GCN --check-prefix=VI --check-prefix=XNACK
+
+; GCN-LABEL: {{^}}test1:
+; NO-XNACK: ; NumSgprs: 1
+; NO-XNACK: ; NumVgprs: 1
+; XNACK: ; NumSgprs: 5
+; XNACK: ; NumVgprs: 1
+define void @test1() {
+entry:
+  call void asm sideeffect "", ""()
+  ret void
+}
+
+; GCN-LABEL: {{^}}test2:
+; NO-XNACK: ; NumSgprs: 8
+; NO-XNACK: ; NumVgprs: 1
+; XNACK: ; NumSgprs: 12
+; XNACK: ; NumVgprs: 1
+define void @test2() {
+entry:
+  call void asm sideeffect "", "~{s7}"()
+  ret void
+}
+
+; GCN-LABEL: {{^}}test3:
+; NO-XNACK: ; NumSgprs: 102
+; NO-XNACK: ; NumVgprs: 1
+; XNACK: ; NumSgprs: 106
+; XNACK: ; NumVgprs: 1
+define void @test3() {
+entry:
+  call void asm sideeffect "", "~{s12},~{s101}"()
+  ret void
+}
+
+; GCN-LABEL: {{^}}test4:
+; NO-XNACK: ; NumSgprs: 1
+; NO-XNACK: ; NumVgprs: 4
+; XNACK: ; NumSgprs: 5
+; XNACK: ; NumVgprs: 4
+define void @test4() {
+entry:
+  call void asm sideeffect "", "~{v3}"()
+  ret void
+}
+
+; GCN-LABEL: {{^}}test5:
+; NO-XNACK: ; NumSgprs: 2
+; NO-XNACK: ; NumVgprs: 256
+; XNACK: ; NumSgprs: 6
+; XNACK: ; NumVgprs: 256
+define void @test5() {
+entry:
+  call void asm sideeffect "", "~{v255},~{s1}"()
+  ret void
+}
+
+; GCN-LABEL: {{^}}test6:
+; NO-XNACK: ; NumSgprs: 102
+; NO-XNACK: ; NumVgprs: 1
+; XNACK: ; NumSgprs: 106
+; XNACK: ; NumVgprs: 1
+define void @test6() {
+entry:
+  call void asm sideeffect "", "~{s12},~{s101}"()
+  ret void
+}
+
+; GCN-LABEL: {{^}}test7:
+; NO-XNACK: ; NumSgprs: 3
+; NO-XNACK: ; NumVgprs: 1
+; XNACK: ; NumSgprs: 5
+; XNACK: ; NumVgprs: 1
+define void @test7() {
+entry:
+  call void asm sideeffect "", "~{vcc},~{vcc_lo}"()
+  ret void
+}
+
+; GCN-LABEL: {{^}}test8:
+; NO-XNACK: ; NumSgprs: 128
+; NO-XNACK: ; NumVgprs: 1
+; XNACK: ; NumSgprs: 132
+; XNACK: ; NumVgprs: 1
+define void @test8() {
+entry:
+  call void asm sideeffect "", "~{exec},~{exec_hi}"()
+  ret void
+}
+
+; GCN-LABEL: {{^}}test9:
+; NO-XNACK-CI: ; NumSgprs: 5
+; NO-XNACK-VI: ; NumSgprs: 7
+; NO-XNACK: ; NumVgprs: 1
+; XNACK: ; NumSgprs: 7
+; XNACK: ; NumVgprs: 1
+define void @test9() {
+entry:
+  call void asm sideeffect "", "~{flat_scratch_lo}"()
+  ret void
+}
+
+; GCN-LABEL: {{^}}test10:
+; NO-XNACK-CI: ; NumSgprs: 5
+; NO-XNACK-VI: ; NumSgprs: 7
+; NO-XNACK: ; NumVgprs: 1
+; XNACK: ; NumSgprs: 7
+; XNACK: ; NumVgprs: 1
+define void @test10() {
+entry:
+  call void asm sideeffect "", "~{flat_scratch}"()
+  ret void
+}
+
+; GCN-LABEL: {{^}}test11:
+; NO-XNACK: ; NumSgprs: 1
+; NO-XNACK: ; NumVgprs: 1
+; XNACK: ; NumSgprs: 5
+; XNACK: ; NumVgprs: 1
+define void @test11() {
+entry:
+  call void asm sideeffect "", "~{scc},~{m0}"()
+  ret void
+}

test/CodeGen/AMDGPU/spill-alloc-sgpr-init-bug.ll

entry:
   %aptr = getelementptr <256 x i32>, <256 x i32> addrspace(1)* %in, i32 %tid
   %a = load <256 x i32>, <256 x i32> addrspace(1)* %aptr
   call void asm sideeffect "", "~{memory}" ()
   %outptr = getelementptr <256 x i32>, <256 x i32> addrspace(1)* %in, i32 %tid
   store <256 x i32> %a, <256 x i32> addrspace(1)* %outptr
 
 ; mark 128-bit SGPR registers as used so they are unavailable for the
 ; scratch resource descriptor
-  call void asm sideeffect "", "~{SGPR4},~{SGPR8},~{SGPR12},~{SGPR16},~{SGPR20},~{SGPR24},~{SGPR28}" ()
-  call void asm sideeffect "", "~{SGPR32},~{SGPR36},~{SGPR40},~{SGPR44},~{SGPR48},~{SGPR52},~{SGPR56}" ()
-  call void asm sideeffect "", "~{SGPR60},~{SGPR64},~{SGPR68}" ()
+  call void asm sideeffect "", "~{s4},~{s8},~{s12},~{s16},~{s20},~{s24},~{s28}" ()
+  call void asm sideeffect "", "~{s32},~{s36},~{s40},~{s44},~{s48},~{s52},~{s56}" ()
+  call void asm sideeffect "", "~{s60},~{s64},~{s68}" ()
   ret void
 }
 
 declare i32 @llvm.SI.tid() nounwind readnone

test/CodeGen/AMDGPU/spill-scavenge-offset.ll

 entry:
   %lo = call i32 @llvm.amdgcn.mbcnt.lo(i32 -1, i32 0)
   %tid = call i32 @llvm.amdgcn.mbcnt.hi(i32 -1, i32 %lo)
 
   %aptr = getelementptr <1280 x i32>, <1280 x i32> addrspace(1)* %in, i32 %tid
   %a = load <1280 x i32>, <1280 x i32> addrspace(1)* %aptr
 
 ; mark most VGPR registers as used to increase register pressure
-  call void asm sideeffect "", "~{VGPR4},~{VGPR8},~{VGPR12},~{VGPR16},~{VGPR20},~{VGPR24},~{VGPR28},~{VGPR32}" ()
-  call void asm sideeffect "", "~{VGPR36},~{VGPR40},~{VGPR44},~{VGPR48},~{VGPR52},~{VGPR56},~{VGPR60},~{VGPR64}" ()
-  call void asm sideeffect "", "~{VGPR68},~{VGPR72},~{VGPR76},~{VGPR80},~{VGPR84},~{VGPR88},~{VGPR92},~{VGPR96}" ()
-  call void asm sideeffect "", "~{VGPR100},~{VGPR104},~{VGPR108},~{VGPR112},~{VGPR116},~{VGPR120},~{VGPR124},~{VGPR128}" ()
-  call void asm sideeffect "", "~{VGPR132},~{VGPR136},~{VGPR140},~{VGPR144},~{VGPR148},~{VGPR152},~{VGPR156},~{VGPR160}" ()
-  call void asm sideeffect "", "~{VGPR164},~{VGPR168},~{VGPR172},~{VGPR176},~{VGPR180},~{VGPR184},~{VGPR188},~{VGPR192}" ()
-  call void asm sideeffect "", "~{VGPR196},~{VGPR200},~{VGPR204},~{VGPR208},~{VGPR212},~{VGPR216},~{VGPR220},~{VGPR224}" ()
+  call void asm sideeffect "", "~{v4},~{v8},~{v12},~{v16},~{v20},~{v24},~{v28},~{v32}" ()
+  call void asm sideeffect "", "~{v36},~{v40},~{v44},~{v48},~{v52},~{v56},~{v60},~{v64}" ()
+  call void asm sideeffect "", "~{v68},~{v72},~{v76},~{v80},~{v84},~{v88},~{v92},~{v96}" ()
+  call void asm sideeffect "", "~{v100},~{v104},~{v108},~{v112},~{v116},~{v120},~{v124},~{v128}" ()
+  call void asm sideeffect "", "~{v132},~{v136},~{v140},~{v144},~{v148},~{v152},~{v156},~{v160}" ()
+  call void asm sideeffect "", "~{v164},~{v168},~{v172},~{v176},~{v180},~{v184},~{v188},~{v192}" ()
+  call void asm sideeffect "", "~{v196},~{v200},~{v204},~{v208},~{v212},~{v216},~{v220},~{v224}" ()
 
   %outptr = getelementptr <1280 x i32>, <1280 x i32> addrspace(1)* %out, i32 %tid
   store <1280 x i32> %a, <1280 x i32> addrspace(1)* %outptr
 
   ret void
 }
 
 declare i32 @llvm.amdgcn.mbcnt.lo(i32, i32) #1
 declare i32 @llvm.amdgcn.mbcnt.hi(i32, i32) #1
 
 attributes #1 = { nounwind readnone }