Move llvm::Register from CodeGen to MC to correct a layering issue

[dsanders]

Summary

The MC layer uses unsigned as registers in the same way as CodeGen (e.g.
MCRegisterClass::contains(). Move llvm::Register to the MC layer so we can
use it in MC API's too.

[arsenm]

I had been planning on introducing a distinct "MCRegister" or "MCReg" for this purpose (to mostly replace MCPhysReg usage), with promotion from MCRegister->Register being allowed, but not the inverse.

[dsanders]

In D65552#1609520, @arsenm wrote:

I had been planning on introducing a distinct "MCRegister" or "MCReg" for this purpose (to mostly replace MCPhysReg usage), with promotion from MCRegister->Register being allowed, but not the inverse.

I agree with having a class wrapping MCPhysReg but I think MCRegister needs to know about the namespacing, vregs, etc. as there's places in the MC layer API's where unsigned isn't limited to MCPhysReg. Once it knows about all that, I doubt a CodeGen-layer Register class would be different from MCRegister.

MCRegisterClass::contains() is the main example I have at the moment. It's often called with virtual register arguments and returns false for this case. You can check this by adding assert(Reg == (Reg & 0xFFFF)) to this function and you'll see it fail on lots of tests as the vregs are passed in (I originally found it by changing unsigned to MCPhysReg in MCRegisterInfo's API's and found that two AMDGPU tests started failing). It happens because TargetRegisterClass::contains() forwards on to MCRegisterClass::contains().

[arsenm]

In D65552#1609529, @dsanders wrote:

MCRegisterClass::contains() is the main example I have at the moment. It's often called with virtual register arguments and returns false for this case.

This sounds really broken. I would expect that to be an illegal call

[dsanders]

In D65552#1609530, @arsenm wrote:

In D65552#1609529, @dsanders wrote:

MCRegisterClass::contains() is the main example I have at the moment. It's often called with virtual register arguments and returns false for this case.

This sounds really broken. I would expect that to be an illegal call

Are you referring to calling TargetRegisterClass::contains() on vregs to begin with or passing them on to MCRegisterClass::contains?
The former is happening everywhere as far as I can see but we could practically make the latter true.

FWIW, this is the only vregs in MC layer case I've found so far but I'm worried that there are more non-physical registers in the MC objects. Historically there's nothing stopping it and I don't have a very good way to discover it at the moment.

[arsenm]

In D65552#1609531, @dsanders wrote:

In D65552#1609530, @arsenm wrote:

In D65552#1609529, @dsanders wrote:

MCRegisterClass::contains() is the main example I have at the moment. It's often called with virtual register arguments and returns false for this case.

This sounds really broken. I would expect that to be an illegal call

Are you referring to calling TargetRegisterClass::contains() on vregs to begin with or passing them on to MCRegisterClass::contains?
The former is happening everywhere as far as I can see but we could practically make the latter true.

Both. It doesn't make any sense to pass a virtual register to either. For a virtual register you would check the register class from MachineRegisterInfo

[dsanders]

In D65552#1609532, @arsenm wrote:

In D65552#1609531, @dsanders wrote:

In D65552#1609530, @arsenm wrote:

In D65552#1609529, @dsanders wrote:

MCRegisterClass::contains() is the main example I have at the moment. It's often called with virtual register arguments and returns false for this case.

This sounds really broken. I would expect that to be an illegal call

Are you referring to calling TargetRegisterClass::contains() on vregs to begin with or passing them on to MCRegisterClass::contains?
The former is happening everywhere as far as I can see but we could practically make the latter true.

Both. It doesn't make any sense to pass a virtual register to either. For a virtual register you would check the register class from MachineRegisterInfo

I can see some sense to the former as a virtual register is never a member of a register class. It will usually give the intended result for passes that act on physical registers.

At this point, here's my plan for these couple patches:

• Make https://reviews.llvm.org/D65553 independent of this patch so that we have the correct namespacing in Register
• Move the definition of the physical register namespace to a stub MCRegister class to allow us to start checking for vregs/stack-slots in the MC layer. Making an actual MCRegister to replace is a separate piece of work (I have another patch that changes various unsigned to MCPhysReg in MCRegisterInfo and it would make more sense to go along with that)
• Add a if (!isPhysical()) return false to TargetRegisterInfo::contains() to prevent non-physreg's leaking into the MC layer by that method

I don't want to take on sorting out the callers of TargetRegisterInfo::contains() though I think that's going to be a rabbit hole and I'm trying to get back to that SrcOp patch :-) (the reason I looked into this on the way is the implicit cast operator from Register to unsigned can still give some easy routes to accidentally mix up immediates and registers, I was looking into the difficulty of requiring it explicit casts)

Does that sound good?

[arsenm]

LGTM

[dsanders]

I assume the green tick was something phabricator did automatically :-)

[dsanders]

All the patches to replace this are up. Abandoning the original

llvm/include/llvm/CodeGen/GlobalISel/CombinerHelper.h

 /// return true if the MachineInstruction changed and false otherwise.
 //
 //===--------------------------------------------------------------------===//
 
 #ifndef LLVM_CODEGEN_GLOBALISEL_COMBINER_HELPER_H
 #define LLVM_CODEGEN_GLOBALISEL_COMBINER_HELPER_H
 
 #include "llvm/CodeGen/LowLevelType.h"
-#include "llvm/CodeGen/Register.h"
+#include "llvm/MC/Register.h"
 
 namespace llvm {
 
 class GISelChangeObserver;
 class MachineIRBuilder;
 class MachineRegisterInfo;
 class MachineInstr;
 class MachineOperand;

llvm/include/llvm/CodeGen/GlobalISel/RegisterBankInfo.h

 
 #ifndef LLVM_CODEGEN_GLOBALISEL_REGISTERBANKINFO_H
 #define LLVM_CODEGEN_GLOBALISEL_REGISTERBANKINFO_H
 
 #include "llvm/ADT/DenseMap.h"
 #include "llvm/ADT/Hashing.h"
 #include "llvm/ADT/SmallVector.h"
 #include "llvm/ADT/iterator_range.h"
-#include "llvm/CodeGen/Register.h"
+#include "llvm/MC/Register.h"
 #include "llvm/Support/ErrorHandling.h"
 #include <cassert>
 #include <initializer_list>
 #include <memory>
 
 namespace llvm {
 
 class MachineInstr;

llvm/include/llvm/CodeGen/GlobalISel/Utils.h

 /// GlobalISel pipeline.
 //
 //===----------------------------------------------------------------------===//
 
 #ifndef LLVM_CODEGEN_GLOBALISEL_UTILS_H
 #define LLVM_CODEGEN_GLOBALISEL_UTILS_H
 
 #include "llvm/ADT/StringRef.h"
-#include "llvm/CodeGen/Register.h"
+#include "llvm/MC/Register.h"
 
 namespace llvm {
 
 class AnalysisUsage;
 class MachineFunction;
 class MachineInstr;
 class MachineOperand;
 class MachineOptimizationRemarkEmitter;

llvm/include/llvm/CodeGen/MachineOperand.h

 //===-- llvm/CodeGen/MachineOperand.h - MachineOperand class ----*- C++ -*-===//
 //
 // Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
 // See https://llvm.org/LICENSE.txt for license information.
 // SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
 //
 //===----------------------------------------------------------------------===//
 //
 // This file contains the declaration of the MachineOperand class.
 //
 //===----------------------------------------------------------------------===//
 
 #ifndef LLVM_CODEGEN_MACHINEOPERAND_H
 #define LLVM_CODEGEN_MACHINEOPERAND_H
 
 #include "llvm/ADT/DenseMap.h"
-#include "llvm/CodeGen/Register.h"
 #include "llvm/IR/Intrinsics.h"
+#include "llvm/MC/Register.h"
 #include "llvm/Support/DataTypes.h"
 #include "llvm/Support/LowLevelTypeImpl.h"
 #include <cassert>
 
 namespace llvm {
 
 class BlockAddress;
 class ConstantFP;

llvm/include/llvm/CodeGen/SwiftErrorValueTracking.h

 //
 //===----------------------------------------------------------------------===//
 
 #ifndef SWIFTERRORVALUETRACKING_H
 #define SWIFTERRORVALUETRACKING_H
 
 #include "llvm/ADT/DenseMap.h"
 #include "llvm/ADT/SmallVector.h"
-#include "llvm/CodeGen/Register.h"
 #include "llvm/IR/BasicBlock.h"
 #include "llvm/IR/DebugLoc.h"
+#include "llvm/MC/Register.h"
 #include <functional>
 #include <type_traits>
 #include <utility>
 
-
 namespace llvm {
   class Function;
   class MachineBasicBlock;
   class MachineFunction;
   class MachineInstr;
   class TargetInstrInfo;
   class TargetLowering;
 

llvm/include/llvm/MC/Register.h

-//===-- llvm/CodeGen/Register.h ---------------------------------*- C++ -*-===//
+//===-- llvm/MC/Register.h --------------------------------------*- C++ -*-===//
 //
 // Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
 // See https://llvm.org/LICENSE.txt for license information.
 // SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
 //
 //===----------------------------------------------------------------------===//
 
-#ifndef LLVM_CODEGEN_REGISTER_H
-#define LLVM_CODEGEN_REGISTER_H
+#ifndef LLVM_MC_REGISTER_H
+#define LLVM_MC_REGISTER_H
 
 #include <cassert>
 
 namespace llvm {
 
 /// Wrapper class representing virtual and physical registers. Should be passed
 /// by value.
 class Register {
public:
 
   bool isValid() const {
     return Reg != 0;
   }
 };
 
 }
 
-#endif
+#endif // ifndef LLVM_MC_REGISTER_H

llvm/lib/Target/AMDGPU/AMDGPUArgumentUsageInfo.h

 //==- AMDGPUArgumentrUsageInfo.h - Function Arg Usage Info -------*- C++ -*-==//
 //
 // Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
 // See https://llvm.org/LICENSE.txt for license information.
 // SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
 //
 //===----------------------------------------------------------------------===//
 
 #ifndef LLVM_LIB_TARGET_AMDGPU_AMDGPUARGUMENTUSAGEINFO_H
 #define LLVM_LIB_TARGET_AMDGPU_AMDGPUARGUMENTUSAGEINFO_H
 
 #include "llvm/ADT/DenseMap.h"
-#include "llvm/CodeGen/Register.h"
 #include "llvm/IR/Function.h"
+#include "llvm/MC/Register.h"
 #include "llvm/Pass.h"
 
 namespace llvm {
 
 class Function;
 class raw_ostream;
 class GCNSubtarget;
 class TargetMachine;

llvm/lib/Target/AMDGPU/AMDGPUInstructionSelector.h

 
 #ifndef LLVM_LIB_TARGET_AMDGPU_AMDGPUINSTRUCTIONSELECTOR_H
 #define LLVM_LIB_TARGET_AMDGPU_AMDGPUINSTRUCTIONSELECTOR_H
 
 #include "AMDGPU.h"
 #include "AMDGPUArgumentUsageInfo.h"
 #include "llvm/ADT/ArrayRef.h"
 #include "llvm/ADT/SmallVector.h"
-#include "llvm/CodeGen/Register.h"
 #include "llvm/CodeGen/GlobalISel/InstructionSelector.h"
 #include "llvm/IR/InstrTypes.h"
+#include "llvm/MC/Register.h"
 
 namespace {
 #define GET_GLOBALISEL_PREDICATE_BITSET
 #define AMDGPUSubtarget GCNSubtarget
 #include "AMDGPUGenGlobalISel.inc"
 #undef GET_GLOBALISEL_PREDICATE_BITSET
 #undef AMDGPUSubtarget
 }

llvm/lib/Target/AMDGPU/AMDGPURegisterBankInfo.h

 //===- AMDGPURegisterBankInfo -----------------------------------*- C++ -*-==//
 //
 // Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
 // See https://llvm.org/LICENSE.txt for license information.
 // SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
 //
 //===----------------------------------------------------------------------===//
 /// \file
 /// This file declares the targeting of the RegisterBankInfo class for AMDGPU.
 /// \todo This should be generated by TableGen.
 //===----------------------------------------------------------------------===//
 
 #ifndef LLVM_LIB_TARGET_AMDGPU_AMDGPUREGISTERBANKINFO_H
 #define LLVM_LIB_TARGET_AMDGPU_AMDGPUREGISTERBANKINFO_H
 
-#include "llvm/CodeGen/Register.h"
 #include "llvm/CodeGen/GlobalISel/RegisterBankInfo.h"
 
 #define GET_REGBANK_DECLARATIONS
 #include "AMDGPUGenRegisterBank.inc"
 #undef GET_REGBANK_DECLARATIONS
 
 namespace llvm {