[llvm] Assert two ValIDs are the same kind before comparing

[leonardchan]

Summary

I suspect the reason for why D134234 was failing sometimes is because "operator<" for a ValID could compare ValIDs of different kinds but have the same non-active values and return an incorrect result. This is an issue if I attempt to store ValIDs of different kinds in an std::map but we compare different "active" values. For example, if I create an std::map and store some ValIDs of kind t_GlobalName, then I insert a ValID of kind t_GlobalID, the current "operator<" will see that one of the operands is a t_GlobalID and compare it against the UIntVal of other items in the map, but the other items in the map don't set UIntVal because they're not t_GlobalIDs, so I compare against a dummy/uninitialized value.

It seems pretty easy to add mixed ValID kinds into an std::map in LLParser, so this just asserts that when doing the comparison that both ValIDs are the same kind.

[leonardchan]

Locally I can assert none of the current llvm tests fail with this in a debug build.

Alternative approaches are just adding another branch that compares the kinds:

bool operator<(const ValID &RHS) const {
  if (Kind != RHS.Kind) return Kind < RHS.Kind;
...

or updating the ValID layout to hide members behind getters/setters that assert the correct kind.

[phosek]

In D134488#3810169, @leonardchan wrote:

Locally I can assert none of the current llvm tests fail with this in a debug build.

Alternative approaches are just adding another branch that compares the kinds:

bool operator<(const ValID &RHS) const {
  if (Kind != RHS.Kind) return Kind < RHS.Kind;
...

or updating the ValID layout to hide members behind getters/setters that assert the correct kind.

I think that asserting the invariant is an improvement regardless of whether we decide to add the comparison later or not.

llvm/include/llvm/AsmParser/LLParser.h

ValID(const ValID &RHS)
         : Kind(RHS.Kind), Loc(RHS.Loc), UIntVal(RHS.UIntVal), FTy(RHS.FTy),
           StrVal(RHS.StrVal), StrVal2(RHS.StrVal2), APSIntVal(RHS.APSIntVal),
           APFloatVal(RHS.APFloatVal), ConstantVal(RHS.ConstantVal),
           NoCFI(RHS.NoCFI) {
       assert(!RHS.ConstantStructElts);
     }
 
     bool operator<(const ValID &RHS) const {
+      assert(Kind == RHS.Kind && "Comparing ValIDs of different kinds");
       if (Kind == t_LocalID || Kind == t_GlobalID)
         return UIntVal < RHS.UIntVal;
       assert((Kind == t_LocalName || Kind == t_GlobalName ||
               Kind == t_ConstantStruct || Kind == t_PackedConstantStruct) &&
              "Ordering not defined for this ValID kind yet");
       return StrVal < RHS.StrVal;
     }
   };