[PATCH] D136043: [CodeGen] Replace CCValAssign::Loc with a discriminated union (NFCI)

[Sergei Barannikov via Phabricator via llvm-commits]

barannikov88 added inline comments.


================
Comment at: llvm/include/llvm/CodeGen/CallingConvLower.h:63
+  // A union discriminated by the kind of the location.
+  union {
+    // When Kind == Register: the register that the value is assigned to.
----------------
tschuett wrote:
> You cannot use `std::variant<Register, unsigned, unsigned>`?
I crafted this patch before LLVM has moved to C++17. I'll try and see what it will look like, thanks for reminding.


================
Comment at: llvm/include/llvm/CodeGen/CallingConvLower.h:63
+  // A union discriminated by the kind of the location.
+  union {
+    // When Kind == Register: the register that the value is assigned to.
----------------
barannikov88 wrote:
> tschuett wrote:
> > You cannot use `std::variant<Register, unsigned, unsigned>`?
> I crafted this patch before LLVM has moved to C++17. I'll try and see what it will look like, thanks for reminding.
I've tried.
`std::variant` allows to remove some assertions and the `Kind` discriminator, some methods become one-liners, and the total patch size is reduced by 20 lines.
However, due to and repeated `unsigned` type and registers passed in arguments as 'unsigned', I have to write weird looking stuff like:
```
  void convertToReg(unsigned RegNo) { Data = Register(RegNo); }

  void convertToMem(unsigned Offset) {
    Data = decltype(Data)(std::in_place_index<LK_Memory>, Offset);
  }
```
which, in my opinion, is more difficult to read than
```
  void convertToReg(unsigned RegNo) {
    Kind = LocKind::Register;
    this->RegNo = RegNo;
  }

  void convertToMem(unsigned Offset) {
    Kind = LocKind::Memory;
    this->Offset = Offset;
  }
```
It would look much prettier if the 'Offset' was 'int64_t' (which I planned to fix next), so that all three types are different and `in_place_index` is unnecessary.
Another small inconvenience is that valuable comment inside the union. Any idea how to port it to`std::variant` variant?

That said, there does not seem to be a huge benefit of using `std::variant` here. But if more people support the idea of using it, I'll update the patch.



Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D136043