[globalisel] Allow SrcOp to convert an APInt and render it as an immediate operand (MO.isImm() == true)

[dsanders]

Summary

This is tested by D61289 but has been pulled into a separate patch at
a reviewers request.

[arsenm]

For the use cases, I think this should just be int64_t instead of APInt

[dsanders]

In D61321#1484604, @arsenm wrote:

For the use cases, I think this should just be int64_t instead of APInt

int64_t was my first thought for it but I got rather worried about the potential mistakes due to registers being unsigned. For example, is 1 a register or an immediate? It turned out that my worries were moot as the compiler doesn't allow both an unsigned and an int64_t overload anyway (https://godbolt.org/z/zO8LhX)

[arsenm]

In D61321#1485092, @dsanders wrote:

In D61321#1484604, @arsenm wrote:

For the use cases, I think this should just be int64_t instead of APInt

int64_t was my first thought for it but I got rather worried about the potential mistakes due to registers being unsigned. For example, is 1 a register or an immediate? It turned out that my worries were moot as the compiler doesn't allow both an unsigned and an int64_t overload anyway (https://godbolt.org/z/zO8LhX)

In D61321#1485092, @dsanders wrote:

In D61321#1484604, @arsenm wrote:

For the use cases, I think this should just be int64_t instead of APInt

int64_t was my first thought for it but I got rather worried about the potential mistakes due to registers being unsigned. For example, is 1 a register or an immediate? It turned out that my worries were moot as the compiler doesn't allow both an unsigned and an int64_t overload anyway (https://godbolt.org/z/zO8LhX)

I vaguely remember trying to do something like this for SrcOp before, and hit this problem but didn't continue working on it. If you disambiguate the constant type, it works: https://godbolt.org/z/aLyc1T
I don't think using needing to use INT64_C is really worse than having to construct an APInt. I really despise using a plain unsigned for registers, but fixing that would be a hopelessly large project

[paquette]

ehhh

I agree it would be nice to use an int64_t because it matches up with the MachineOperand API. (e.g. getImm() returns an int64_t)

But, OTOH, I can see the benefit of disambiguating the type using an APInt.

Overall, the patch looks fine to me. I'd be fine with either solution. @arsenm, do you feel strongly about this?

[arsenm]

How terrible would it be to use APInt as the interface type, but then store it as int64_t?

[aditya_nandakumar]

In D61321#1546581, @arsenm wrote:

How terrible would it be to use APInt as the interface type, but then store it as int64_t?

That seems like a good compromise to me.

[bogner]

In D61321#1546724, @aditya_nandakumar wrote:

In D61321#1546581, @arsenm wrote:

How terrible would it be to use APInt as the interface type, but then store it as int64_t?

That seems like a good compromise to me.

I don't think I agree. APInt as an interface type won't really save any typing, so what advantage would this compromise have? It's about the same to write SrcOp(APInt(x)) vs SrcOp(int64_t(x)), but the APInt version is more expensive and if we're not actually handling arbitrary precision it's quite a bit more confusing.

[aditya_nandakumar]

In D61321#1546809, @bogner wrote:

In D61321#1546724, @aditya_nandakumar wrote:

In D61321#1546581, @arsenm wrote:

How terrible would it be to use APInt as the interface type, but then store it as int64_t?

That seems like a good compromise to me.

I don't think I agree. APInt as an interface type won't really save any typing, so what advantage would this compromise have? It's about the same to write SrcOp(APInt(x)) vs SrcOp(int64_t(x)), but the APInt version is more expensive and if we're not actually handling arbitrary precision it's quite a bit more confusing.

I think it'd be nice to not worry about picking the wrong overloads. For eg, if you have an unsigned constant and you forget to explicitly cast to int64_t it will still compile (and pick the register version even though we wanted the immediate) - where as if the only way to get immediate is APInt and we forget to cast, it will not compile.

[arsenm]

We could try to be heroic and use something stronger than unsigned for registers

[aditya_nandakumar]

In D61321#1546826, @arsenm wrote:

We could try to be heroic and use something stronger than unsigned for registers

In D61321#1546826, @arsenm wrote:

We could try to be heroic and use something stronger than unsigned for registers

Are you talking about using stronger types for registers just in the Builder interface? What would the usage look like? My big concern (besides the massive change this would take) is how verbose it would be to use the APIs.

[arsenm]

In D61321#1547008, @aditya_nandakumar wrote:

In D61321#1546826, @arsenm wrote:

We could try to be heroic and use something stronger than unsigned for registers

In D61321#1546826, @arsenm wrote:

We could try to be heroic and use something stronger than unsigned for registers

Are you talking about using stronger types for registers just in the Builder interface? What would the usage look like? My big concern (besides the massive change this would take) is how verbose it would be to use the APIs.

I mean everywhere, but that's a huge project. I imagine a struct Register { unsigned X; isVirtual(); isPhysical() }

[aemerson]

In D61321#1547021, @arsenm wrote:

In D61321#1547008, @aditya_nandakumar wrote:

In D61321#1546826, @arsenm wrote:

We could try to be heroic and use something stronger than unsigned for registers

In D61321#1546826, @arsenm wrote:

We could try to be heroic and use something stronger than unsigned for registers

Are you talking about using stronger types for registers just in the Builder interface? What would the usage look like? My big concern (besides the massive change this would take) is how verbose it would be to use the APIs.

I mean everywhere, but that's a huge project. I imagine a struct Register { unsigned X; isVirtual(); isPhysical() }

I'm sure Aditya could cook up one of his clang plugins to automate it.

[dsanders]

In D61321#1546826, @arsenm wrote:

We could try to be heroic and use something stronger than unsigned for registers

Are you talking about using stronger types for registers just in the Builder interface? What would the usage look like? My big concern (besides the massive change this would take) is how verbose it would be to use the APIs.

I mean everywhere, but that's a huge project. I imagine a struct Register { unsigned X; isVirtual(); isPhysical() }

FWIW, that would be my preferred solution if it weren't for the sheer amount of time required to achieve it. It's always felt wrong that we use unsigned to represent registers.

[arsenm]

In D61321#1547076, @dsanders wrote:

In D61321#1546826, @arsenm wrote:

We could try to be heroic and use something stronger than unsigned for registers

Are you talking about using stronger types for registers just in the Builder interface? What would the usage look like? My big concern (besides the massive change this would take) is how verbose it would be to use the APIs.

I mean everywhere, but that's a huge project. I imagine a struct Register { unsigned X; isVirtual(); isPhysical() }

FWIW, that would be my preferred solution if it weren't for the sheer amount of time required to achieve it. It's always felt wrong that we use unsigned to represent registers.

I tried prototyping it, and it seems to not be entirely infeasible: D63496

I think managing the migration inside GlobalISel should be manageable enough to avoid having an unsigned overload of SrcOp

[dsanders]

I'm currently rebasing this patch switching APInt to uint64_t and one thing I'm a bit worried about is the case where the implicit conversion from Register to unsigned causes registers to be passed where immediates are expected. At the moment our plan guards against the case where Register is correctly used but not against trivial misuse like:

unsigned Reg = Register();
SrcOp(Reg)

and with the current unsigned constructor, this:

SrcOp(1)

will be treated as a register.

I had a look into the feasibility of eliminating the implicit conversions and that's way too big a task at the moment to block this on fixing that (I'll post some patches that propagate Register a bit more tomorrow, they were a side-effect of looking into it). I have a potential alternative though. We can add some deleted constructors like in https://godbolt.org/z/0zQOJX. The downside of this is plain constants like 0 won't work, you need to use 0ul. Does that limitation sound ok if it makes mixing up registers and immediates harder?

[dsanders]

In D61321#1611610, @dsanders wrote:

I'm currently rebasing this patch switching APInt to uint64_t and one thing I'm a bit worried about is the case where the implicit conversion from Register to unsigned causes registers to be passed where immediates are expected. At the moment our plan guards against the case where Register is correctly used but not against trivial misuse like:

unsigned Reg = Register();
SrcOp(Reg)

and with the current unsigned constructor, this:

SrcOp(1)

will be treated as a register.

I had a look into the feasibility of eliminating the implicit conversions and that's way too big a task at the moment to block this on fixing that (I'll post some patches that propagate Register a bit more tomorrow, they were a side-effect of looking into it). I have a potential alternative though. We can add some deleted constructors like in https://godbolt.org/z/0zQOJX. The downside of this is plain constants like 0 won't work, you need to use 0ul. Does that limitation sound ok if it makes mixing up registers and immediates harder?

Of course, the moment I post I notice a case I'd missed. AArch64::WZR is an unsigned, we'd have to pass it as Register(AArch64::WZR) to avoid hitting the deleted constructor

[arsenm]

In D61321#1611613, @dsanders wrote:

In D61321#1611610, @dsanders wrote:

I'm currently rebasing this patch switching APInt to uint64_t and one thing I'm a bit worried about is the case where the implicit conversion from Register to unsigned causes registers to be passed where immediates are expected. At the moment our plan guards against the case where Register is correctly used but not against trivial misuse like:

unsigned Reg = Register();
SrcOp(Reg)

and with the current unsigned constructor, this:

SrcOp(1)

will be treated as a register.

I had a look into the feasibility of eliminating the implicit conversions and that's way too big a task at the moment to block this on fixing that (I'll post some patches that propagate Register a bit more tomorrow, they were a side-effect of looking into it). I have a potential alternative though. We can add some deleted constructors like in https://godbolt.org/z/0zQOJX. The downside of this is plain constants like 0 won't work, you need to use 0ul. Does that limitation sound ok if it makes mixing up registers and immediates harder?

Of course, the moment I post I notice a case I'd missed. AArch64::WZR is an unsigned, we'd have to pass it as Register(AArch64::WZR) to avoid hitting the deleted constructor

This is already an issue I wasn’t able to come up with a good solution for independent of adding the new imm SrcOp

[dsanders]

In D61321#1611626, @arsenm wrote:

In D61321#1611613, @dsanders wrote:

In D61321#1611610, @dsanders wrote:

I'm currently rebasing this patch switching APInt to uint64_t and one thing I'm a bit worried about is the case where the implicit conversion from Register to unsigned causes registers to be passed where immediates are expected. At the moment our plan guards against the case where Register is correctly used but not against trivial misuse like:

unsigned Reg = Register();
SrcOp(Reg)

and with the current unsigned constructor, this:

SrcOp(1)

will be treated as a register.

I had a look into the feasibility of eliminating the implicit conversions and that's way too big a task at the moment to block this on fixing that (I'll post some patches that propagate Register a bit more tomorrow, they were a side-effect of looking into it). I have a potential alternative though. We can add some deleted constructors like in https://godbolt.org/z/0zQOJX. The downside of this is plain constants like 0 won't work, you need to use 0ul. Does that limitation sound ok if it makes mixing up registers and immediates harder?

Of course, the moment I post I notice a case I'd missed. AArch64::WZR is an unsigned, we'd have to pass it as Register(AArch64::WZR) to avoid hitting the deleted constructor

This is already an issue I wasn’t able to come up with a good solution for independent of adding the new imm SrcOp

The main thing I need to establish to update and land a version of this patch is what our preference is. Do we prefer?:

1. having immediates wrapped in APInt and registers being unsigned/Register. This is more compatible with the current code.
2. immediates being APInt/uint64_t/int64_t, immediate constants being 0ul, registers being wrapped in Register, and unsigned being invalid. Later when the implicit Register->unsigned conversion is deleted re-allow unsigned as immediates
3. something else

1. is less invasive to the current code and is much easier to land but 2. better aligns with our long term thoughts on eliminating unsigned for registers. My opinion depends on difficulty with 2 being my first choice and 1 being my fallback if changing unsigned->Register significantly exceeds my expectations in terms of how much porting work to the existing code is needed.

What are your preferences?

[arsenm]

I would vote for option 2

[aditya_nandakumar]

If option 2 is possible then go for it. If it's too much for work for now, then fallback to 1.

[paquette]

Since option 2 is more in line with the long-term vision, I think it would be better to go down that route. Better to do it sooner than later.

[dsanders]

Looks like we have agreement on option 2. Thanks. I'll update the patch in that direction

[dsanders]

Rebase and update to use uint64_t (and ban registers in unsigned)

[arsenm]

llvm/include/llvm/CodeGen/GlobalISel/MachineIRBuilder.h

119

Usually arbitrary immediates use signed int64_t, as MachineOperand does

212

Ditto

[dsanders]

• uint64_t -> int64_t

[arsenm]

LGTM

llvm/include/llvm/CodeGen/GlobalISel/MachineIRBuilder.h

public:
   }
 
   DstType getDstOpKind() const { return Ty; }
 
 private:
   DstType Ty;
 };
 
 class SrcOp {
   union {
     MachineInstrBuilder SrcMIB;
     unsigned Reg;
     CmpInst::Predicate Pred;
+    APInt Imm;
   };
 
 public:
-  enum class SrcType { Ty_Reg, Ty_MIB, Ty_Predicate };
+  enum class SrcType { Ty_Reg, Ty_MIB, Ty_Predicate, Ty_Imm };
   SrcOp(unsigned R) : Reg(R), Ty(SrcType::Ty_Reg) {}
   SrcOp(const MachineOperand &Op) : Reg(Op.getReg()), Ty(SrcType::Ty_Reg) {}
   SrcOp(const MachineInstrBuilder &MIB) : SrcMIB(MIB), Ty(SrcType::Ty_MIB) {}
   SrcOp(const CmpInst::Predicate P) : Pred(P), Ty(SrcType::Ty_Predicate) {}
+  SrcOp(const APInt &V) : Imm(V), Ty(SrcType::Ty_Imm) {}
+  SrcOp(const SrcOp &Other) {
+    // copy-and-swap is apparently preferred but doesn't actually help me reduce
+    // the code duplication in this case as the swap ends up requiring the
+    // duplicate code instead;
+    operator=(Other);
+  }
+  ~SrcOp() {}
+  SrcOp& operator=(const SrcOp &Other) {
+    Ty = Other.Ty;
+    switch (Ty) {
+    case SrcType::Ty_Predicate:
+      Pred = Other.Pred;
+      break;
+    case SrcType::Ty_Reg:
+      Reg = Other.Reg;
+      break;
+    case SrcType::Ty_MIB:
+      SrcMIB = Other.SrcMIB;
+      break;
+    case SrcType::Ty_Imm:
+      Imm = Other.Imm;
+      break;
+    }
+    return *this;
+  }
 
   void addSrcToMIB(MachineInstrBuilder &MIB) const {
     switch (Ty) {
     case SrcType::Ty_Predicate:
       MIB.addPredicate(Pred);
       break;
     case SrcType::Ty_Reg:
       MIB.addUse(Reg);
       break;
     case SrcType::Ty_MIB:
       MIB.addUse(SrcMIB->getOperand(0).getReg());
       break;
+    case SrcType::Ty_Imm:
+      MIB.addImm(Imm.getSExtValue());
+      break;
     }
   }
 
   LLT getLLTTy(const MachineRegisterInfo &MRI) const {
     switch (Ty) {
     case SrcType::Ty_Predicate:
+    case SrcType::Ty_Imm:
       llvm_unreachable("Not a register operand");
     case SrcType::Ty_Reg:
       return MRI.getType(Reg);
     case SrcType::Ty_MIB:
       return MRI.getType(SrcMIB->getOperand(0).getReg());
     }
     llvm_unreachable("Unrecognised SrcOp::SrcType enum");
   }
 
   unsigned getReg() const {
     switch (Ty) {
     case SrcType::Ty_Predicate:
+    case SrcType::Ty_Imm:
       llvm_unreachable("Not a register operand");
     case SrcType::Ty_Reg:
       return Reg;
     case SrcType::Ty_MIB:
       return SrcMIB->getOperand(0).getReg();
     }
     llvm_unreachable("Unrecognised SrcOp::SrcType enum");
   }
 
   CmpInst::Predicate getPredicate() const {
     switch (Ty) {
     case SrcType::Ty_Predicate:
       return Pred;
     default:
       llvm_unreachable("Not a register operand");
     }
   }
 
+  APInt getImm() const {
+    switch (Ty) {
+    case SrcType::Ty_Imm:
+      return Imm;
+    default:
+      llvm_unreachable("Not an immediate");
+    }
+  }
+
   SrcType getSrcOpKind() const { return Ty; }
 
 private:
   SrcType Ty;
 };
 
 class FlagsOp {
   Optional<unsigned> Flags;