[CodeGen] Replace CCValAssign::Loc with std::variant (NFCI)

[barannikov88]

Summary

The motivation behind this change is as follows.
Targets with stack growing up (there are no such in-tree targets) pass
arguments at negative offsets relative to the stack pointer. This makes
it hard to use the generic value assigner because CCValAssign stores the
offset as an unsigned integer, which is then zero-extended when
converted to int64_t, e.g. when passing to CreateFixedObject. This
results in conversion of, for example, -4 into 4294967292, which is not
desired.

While it is possible to insert a cast to int before passing the result
of getLocMemOffset into CreateFixedObject in backend code, this is
error-prone, and some uses of getLocMemOffset are located in
places common to all backends (e.g. CallLowering::handleAssignments).

That said, I wanted to change the type of the memory offset from
unsigned to int64_t (this would be consistent with other places
where stack offsets are used). However, the Loc field which stores the
offset is shared between three different kinds of the location:
register, memory, and "pending". Storing a register number as int64_t
does not seem right (there are Register and MCRegister for this), so
I did the most straightforward change - replaced the Loc field with
std::variant.

The main change that changes the type of the memory offset from
unsigned to int64_t will be in a follow-up patch to simplify the
review.

[barannikov88]

llvm/include/llvm/CodeGen/CallingConvLower.h

76–77

This constructor was added because the default constructor used in get* methods has become implicitly-deleted (due to non-trivial constructor of Register in the union). This also reduces code duplication a bit.

[barannikov88]

Editorial suggestions are appreciated.

[barannikov88]

Revert unintended change

[barannikov88]

ping

[barannikov88]

Rebase

[koakuma]

Looks okay to me, at least on the SPARC side.
For the CodeGen changes I'm not seeing anything obviously wrong, but it'd be better if someone else also review that part since I'm not very familiar with its workings.

llvm/lib/Target/Sparc/SparcISelLowering.cpp

1155–1156

Minor nit but it'd be better if this line is removed.

[barannikov88]

Remove empty line

[barannikov88]

Rebase

[barannikov88]

Ping
I don't know who to ask for review, this part of CodeGen is not changed frequently. The last non-refactoring changes were made two years ago.

[tschuett]

llvm/include/llvm/CodeGen/CallingConvLower.h

56

You cannot use std::variant<Register, unsigned, unsigned>?

[barannikov88]

llvm/include/llvm/CodeGen/CallingConvLower.h

56

I crafted this patch before LLVM has moved to C++17. I'll try and see what it will look like, thanks for reminding.

56

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.

[tschuett]

I cannot stop you, but I prefer variant over unions. Does it become easier, if you introduce the int64_t in this patch? You could put the documentation above the variant?

[barannikov88]

In D136043#4003356, @tschuett wrote:

Does it become easier, if you introduce the int64_t in this patch?

Changing the type to int64_t is not a local change, it will bloat the patch considerably and make it harder to review.
I also don't want to change the type to int64_t before this patch, because unsigned Loc; is shared between Register and Memory and Pending variants, and changing it to int64_t does not look right.
What do you say if I ping you after the second patch lands so that you can exercise std::variant here?

[tschuett]

Sure.

[barannikov88]

Ah, I don't have to change the interfaces right away, I only need to change the storage type. Just a sec...

[barannikov88]

Use std::variant.

[barannikov88]

@tschuett
PTAL. This is the first time I ever used std::variant, so it might not be very canonical.
Any other comments are also welcome, including my poor English.

[tschuett]

I am unsure about the value of LockKind.

llvm/include/llvm/CodeGen/CallingConvLower.h

123

Here you could also use std::holds_variant<unsigned>(Data).

130

Or std::get<int>(Data).

[barannikov88]

In D136043#4003380, @tschuett wrote:

I am unsure about the value of LockKind.

Could you elaborate?

llvm/include/llvm/CodeGen/CallingConvLower.h

123

Yes, thanks.
I'd keep it with symbolic constant: it protects from bugs if the storage type changes and is arguably more readable.

[tschuett]

Either you have to keep LockKind in-sync with the variants or you have to keep the variants in the functions in-sync. Either is fine by me.

[barannikov88]

Address review comments

[barannikov88]

In D136043#4003403, @tschuett wrote:

Either you have to keep LockKind in-sync with the variants or you have to keep the variants in the functions in-sync. Either is fine by me.

I see now, thanks. That is actually a good point that I didn't think of.
I removed the enum and updated methods to use types as you suggested in inline comments.

[barannikov88]

Remove unnecessarily introduced extra newlines.

[barannikov88]

Ping...

[nikic]

LGTM

llvm/include/llvm/CodeGen/CallingConvLower.h

VExt,      // The value is vector-widened in the location.
                // FIXME: Not implemented yet. Code that uses AExt to mean
                // vector-widen should be fixed to use VExt instead.
     FPExt,     // The floating-point value is fp-extended in the location.
     Indirect   // The location contains pointer to the value.
     // TODO: a subset of the value is in the location.
   };
 
 private:
+  // Holds one of:
+  // - the register that the value is assigned to;
+  // - the memory offset at which the value resides;
+  // - additional information about pending location; the exact interpretation
+  //   of the data is target-dependent.
+  std::variant<Register, int64_t, unsigned> Data;
+
   /// ValNo - This is the value number being assigned (e.g. an argument number).
   unsigned ValNo;
 
-  /// Loc is either a stack offset or a register number.
-  unsigned Loc;
-
-  /// isMem - True if this is a memory loc, false if it is a register loc.
-  unsigned isMem : 1;
-
   /// isCustom - True if this arg/retval requires special handling.
   unsigned isCustom : 1;
 
   /// Information about how the value is assigned.
   LocInfo HTP : 6;
 
   /// ValVT - The type of the value being assigned.
   MVT ValVT;
 
   /// LocVT - The type of the location being assigned to.
   MVT LocVT;
-public:
 
-  static CCValAssign getReg(unsigned ValNo, MVT ValVT,
-                            unsigned RegNo, MVT LocVT,
-                            LocInfo HTP) {
-    CCValAssign Ret;
-    Ret.ValNo = ValNo;
-    Ret.Loc = RegNo;
-    Ret.isMem = false;
-    Ret.isCustom = false;
-    Ret.HTP = HTP;
-    Ret.ValVT = ValVT;
-    Ret.LocVT = LocVT;
-    return Ret;
+  CCValAssign(LocInfo HTP, unsigned ValNo, MVT ValVT, MVT LocVT, bool IsCustom)
+      : ValNo(ValNo), isCustom(IsCustom), HTP(HTP), ValVT(ValVT), LocVT(LocVT) {
   }
 
-  static CCValAssign getCustomReg(unsigned ValNo, MVT ValVT,
-                                  unsigned RegNo, MVT LocVT,
-                                  LocInfo HTP) {
-    CCValAssign Ret;
-    Ret = getReg(ValNo, ValVT, RegNo, LocVT, HTP);
-    Ret.isCustom = true;
+public:
+  static CCValAssign getReg(unsigned ValNo, MVT ValVT, unsigned RegNo,
+                            MVT LocVT, LocInfo HTP, bool IsCustom = false) {
+    CCValAssign Ret(HTP, ValNo, ValVT, LocVT, IsCustom);
+    Ret.Data = Register(RegNo);
     return Ret;
   }
 
-  static CCValAssign getMem(unsigned ValNo, MVT ValVT,
-                            unsigned Offset, MVT LocVT,
-                            LocInfo HTP) {
-    CCValAssign Ret;
-    Ret.ValNo = ValNo;
-    Ret.Loc = Offset;
-    Ret.isMem = true;
-    Ret.isCustom = false;
-    Ret.HTP = HTP;
-    Ret.ValVT = ValVT;
-    Ret.LocVT = LocVT;
-    return Ret;
+  static CCValAssign getCustomReg(unsigned ValNo, MVT ValVT, unsigned RegNo,
+                                  MVT LocVT, LocInfo HTP) {
+    return getReg(ValNo, ValVT, RegNo, LocVT, HTP, /*IsCustom=*/true);
   }
 
-  static CCValAssign getCustomMem(unsigned ValNo, MVT ValVT,
-                                  unsigned Offset, MVT LocVT,
-                                  LocInfo HTP) {
-    CCValAssign Ret;
-    Ret = getMem(ValNo, ValVT, Offset, LocVT, HTP);
-    Ret.isCustom = true;
+  static CCValAssign getMem(unsigned ValNo, MVT ValVT, unsigned Offset,
+                            MVT LocVT, LocInfo HTP, bool IsCustom = false) {
+    CCValAssign Ret(HTP, ValNo, ValVT, LocVT, IsCustom);
+    Ret.Data = int64_t(Offset);
     return Ret;
   }
 
-  // There is no need to differentiate between a pending CCValAssign and other
-  // kinds, as they are stored in a different list.
+  static CCValAssign getCustomMem(unsigned ValNo, MVT ValVT, unsigned Offset,
+                                  MVT LocVT, LocInfo HTP) {
+    return getMem(ValNo, ValVT, Offset, LocVT, HTP, /*IsCustom=*/true);
+  }
+
   static CCValAssign getPending(unsigned ValNo, MVT ValVT, MVT LocVT,
                                 LocInfo HTP, unsigned ExtraInfo = 0) {
-    return getReg(ValNo, ValVT, ExtraInfo, LocVT, HTP);
+    CCValAssign Ret(HTP, ValNo, ValVT, LocVT, false);
+    Ret.Data = ExtraInfo;
+    return Ret;
   }
 
-  void convertToReg(unsigned RegNo) {
-    Loc = RegNo;
-    isMem = false;
-  }
+  void convertToReg(unsigned RegNo) { Data = Register(RegNo); }
 
-  void convertToMem(unsigned Offset) {
-    Loc = Offset;
-    isMem = true;
-  }
+  void convertToMem(unsigned Offset) { Data = int64_t(Offset); }
 
   unsigned getValNo() const { return ValNo; }
   MVT getValVT() const { return ValVT; }
 
-  bool isRegLoc() const { return !isMem; }
-  bool isMemLoc() const { return isMem; }
+  bool isRegLoc() const { return std::holds_alternative<Register>(Data); }
+  bool isMemLoc() const { return std::holds_alternative<int64_t>(Data); }
+  bool isPendingLoc() const { return std::holds_alternative<unsigned>(Data); }
 
   bool needsCustom() const { return isCustom; }
 
-  Register getLocReg() const { assert(isRegLoc()); return Loc; }
-  unsigned getLocMemOffset() const { assert(isMemLoc()); return Loc; }
-  unsigned getExtraInfo() const { return Loc; }
+  Register getLocReg() const { return std::get<Register>(Data); }
+  unsigned getLocMemOffset() const { return std::get<int64_t>(Data); }
+  unsigned getExtraInfo() const { return std::get<unsigned>(Data); }
+
   MVT getLocVT() const { return LocVT; }
 
   LocInfo getLocInfo() const { return HTP; }
   bool isExtInLoc() const {
     return (HTP == AExt || HTP == SExt || HTP == ZExt);
   }
 
   bool isUpperBitsInLoc() const {
     return HTP == AExtUpper || HTP == SExtUpper || HTP == ZExtUpper;

llvm/lib/CodeGen/CallingConvLower.cpp

for (unsigned I = NumLocs, E = Locs.size(); I != E; ++I)
     if (Locs[I].isRegLoc())
       Regs.push_back(MCPhysReg(Locs[I].getLocReg()));
 
   // Clear the assigned values and stack memory. We leave the registers marked
   // as allocated so that future queries don't return the same registers, i.e.
   // when i64 and f64 are both passed in GPRs.
   StackOffset = SavedStackOffset;
   MaxStackArgAlign = SavedMaxStackArgAlign;
-  Locs.resize(NumLocs);
+  Locs.truncate(NumLocs);
 }
 
 void CCState::analyzeMustTailForwardedRegisters(
     SmallVectorImpl<ForwardedRegister> &Forwards, ArrayRef<MVT> RegParmTypes,
     CCAssignFn Fn) {
   // Oftentimes calling conventions will not user register parameters for
   // variadic functions, so we need to assume we're not variadic so that we get
   // all the registers that might be used in a non-variadic call.
bool CCState::resultsCompatible(CallingConv::ID CalleeCC,
   SmallVector<CCValAssign, 4> RVLocs1;
   CCState CCInfo1(CalleeCC, false, MF, RVLocs1, C);
   CCInfo1.AnalyzeCallResult(Ins, CalleeFn);
 
   SmallVector<CCValAssign, 4> RVLocs2;
   CCState CCInfo2(CallerCC, false, MF, RVLocs2, C);
   CCInfo2.AnalyzeCallResult(Ins, CallerFn);
 
-  if (RVLocs1.size() != RVLocs2.size())
+  auto AreCompatible = [](const CCValAssign &Loc1, const CCValAssign &Loc2) {
+    assert(!Loc1.isPendingLoc() && !Loc2.isPendingLoc() &&
+           "The location must have been decided by now");
+    // Must fill the same part of their locations.
+    if (Loc1.getLocInfo() != Loc2.getLocInfo())
       return false;
-  for (unsigned I = 0, E = RVLocs1.size(); I != E; ++I) {
-    const CCValAssign &Loc1 = RVLocs1[I];
-    const CCValAssign &Loc2 = RVLocs2[I];
-
-    if ( // Must both be in registers, or both in memory
-        Loc1.isRegLoc() != Loc2.isRegLoc() ||
-        // Must fill the same part of their locations
-        Loc1.getLocInfo() != Loc2.getLocInfo() ||
-        // Memory offset/register number must be the same
-        Loc1.getExtraInfo() != Loc2.getExtraInfo())
-      return false;
-  }
-  return true;
+    // Must both be in the same registers, or both in memory at the same offset.
+    if (Loc1.isRegLoc() && Loc2.isRegLoc())
+      return Loc1.getLocReg() == Loc2.getLocReg();
+    if (Loc1.isMemLoc() && Loc2.isMemLoc())
+      return Loc1.getLocMemOffset() == Loc2.getLocMemOffset();
+    llvm_unreachable("Unknown location kind");
+  };
+
+  return std::equal(RVLocs1.begin(), RVLocs1.end(), RVLocs2.begin(),
+                    RVLocs2.end(), AreCompatible);
 }

llvm/lib/Target/Sparc/SparcISelLowering.cpp

 // as integers when calling a varargs function. This does not apply to the
 // fixed arguments that are part of the function's prototype.
 //
 // This function post-processes a CCValAssign array created by
 // AnalyzeCallOperands().
 static void fixupVariableFloatArgs(SmallVectorImpl<CCValAssign> &ArgLocs,
                                    ArrayRef<ISD::OutputArg> Outs) {
   for (unsigned i = 0, e = ArgLocs.size(); i != e; ++i) {
-    const CCValAssign &VA = ArgLocs[i];
+    CCValAssign &VA = ArgLocs[i];
     MVT ValTy = VA.getLocVT();
     // FIXME: What about f32 arguments? C promotes them to f64 when calling
     // varargs functions.
     if (!VA.isRegLoc() || (ValTy != MVT::f64 && ValTy != MVT::f128))
       continue;
     // The fixed arguments to a varargs function still go in FP registers.
     if (Outs[VA.getValNo()].IsFixed)
       continue;
 
     // This floating point argument should be reassigned.
-    CCValAssign NewVA;
-
     // Determine the offset into the argument array.
     Register firstReg = (ValTy == MVT::f64) ? SP::D0 : SP::Q0;
     unsigned argSize  = (ValTy == MVT::f64) ? 8 : 16;
     unsigned Offset = argSize * (VA.getLocReg() - firstReg);
     assert(Offset < 16*8 && "Offset out of range, bad register enum?");
 
     if (Offset < 6*8) {
       // This argument should go in %i0-%i5.
       unsigned IReg = SP::I0 + Offset/8;
       if (ValTy == MVT::f64)
         // Full register, just bitconvert into i64.
-        NewVA = CCValAssign::getReg(VA.getValNo(), VA.getValVT(),
-                                    IReg, MVT::i64, CCValAssign::BCvt);
+        VA = CCValAssign::getReg(VA.getValNo(), VA.getValVT(), IReg, MVT::i64,
+                                 CCValAssign::BCvt);
       else {
         assert(ValTy == MVT::f128 && "Unexpected type!");
         // Full register, just bitconvert into i128 -- We will lower this into
         // two i64s in LowerCall_64.
-        NewVA = CCValAssign::getCustomReg(VA.getValNo(), VA.getValVT(),
-                                          IReg, MVT::i128, CCValAssign::BCvt);
+        VA = CCValAssign::getCustomReg(VA.getValNo(), VA.getValVT(), IReg,
+                                       MVT::i128, CCValAssign::BCvt);
       }
     } else {
       // This needs to go to memory, we're out of integer registers.
-      NewVA = CCValAssign::getMem(VA.getValNo(), VA.getValVT(),
-                                  Offset, VA.getLocVT(), VA.getLocInfo());
+      VA = CCValAssign::getMem(VA.getValNo(), VA.getValVT(), Offset,
+                               VA.getLocVT(), VA.getLocInfo());
     }
-    ArgLocs[i] = NewVA;
   }
 }
 
 // Lower a call for the 64-bit ABI.
 SDValue
 SparcTargetLowering::LowerCall_64(TargetLowering::CallLoweringInfo &CLI,
                                   SmallVectorImpl<SDValue> &InVals) const {
   SelectionDAG &DAG = CLI.DAG;