[PATCH] [Review Request] Improve getVTList() in SelectionDAG

[Owen Anderson]

Some feedback:

> +  /// getEmptyKey() - A private constructor that returns an unknown that is
> +  /// not equal to the tombstone key or SDVTListInfo().
> +  static SDVTListInfo getEmptyKey() {
> +    SDVTListInfo SDVTInfo;
> +    SDVTInfo.NumVTs = 0;
> +    return SDVTInfo;
> +  }
> +
> +  /// getTombstoneKey() - A private constructor that returns an unknown that
> +  /// is not equal to the empty key or SDVTListInfo().
> +  static SDVTListInfo getTombstoneKey() {
> +    SDVTListInfo SDVTInfo;
> +    SDVTInfo.NumVTs = 0;
> +    return SDVTInfo;
> +  }

I don’t think this is a valid implementation of the DenseMapInfo callbacks.  getEmptyKey() and getTombstoneKey() have to return values that don’t compare equal to each other.

> +  const EVT pVTs[2] = {VT1, VT2};
> +  return getVTList(pVTs, 2);

How does the lifetime of this array work out?  It looks like SDVTListInfo captures the pointer to the EVT array, which was stack allocated.  After this method returns, you’re going to have a dangling pointer in the SDVTListInfo.

—Owen



On Oct 10, 2013, at 6:58 AM, Wan, Xiaofei <xiaofei.wan at intel.com> wrote:

> Owen:
> 
> Could you have some time to look at this patch, many thanks.
> 
> Thanks
> Wan Xiaofei
> 
> -----Original Message-----
> From: Wan, Xiaofei [mailto:xiaofei.wan at intel.com] 
> Sent: Wednesday, October 09, 2013 9:09 PM
> To: resistor at mac.com; Wan, Xiaofei
> Cc: llvm-commits at cs.uiuc.edu
> Subject: Re: [PATCH] [Review Request] Improve getVTList() in SelectionDAG
> 
>  Change to use the existing data structure SmallDenseMap.
> 
> Hi resistor,
> 
> http://llvm-reviews.chandlerc.com/D1127
> 
> CHANGE SINCE LAST DIFF
>  http://llvm-reviews.chandlerc.com/D1127?vs=2766&id=4756#toc
> 
> Files:
>  include/llvm/CodeGen/SelectionDAG.h
>  lib/CodeGen/SelectionDAG/SelectionDAG.cpp
> 
> Index: include/llvm/CodeGen/SelectionDAG.h
> ===================================================================
> --- include/llvm/CodeGen/SelectionDAG.h
> +++ include/llvm/CodeGen/SelectionDAG.h
> @@ -58,6 +58,42 @@
>   static void createNode(const SDNode &);  };
> 
> +
> +struct SDVTListInfo {
> +  const EVT *pVTs;
> +  unsigned int NumVTs;
> +  
> +  friend struct DenseMapInfo<SDVTListInfo>;
> +
> +  /// getEmptyKey() - A private constructor that returns an unknown 
> + that is  /// not equal to the tombstone key or SDVTListInfo().
> +  static SDVTListInfo getEmptyKey() {
> +    SDVTListInfo SDVTInfo;
> +    SDVTInfo.NumVTs = 0;
> +    return SDVTInfo;
> +  }
> +
> +  /// getTombstoneKey() - A private constructor that returns an unknown 
> + that  /// is not equal to the empty key or SDVTListInfo().
> +  static SDVTListInfo getTombstoneKey() {
> +    SDVTListInfo SDVTInfo;
> +    SDVTInfo.NumVTs = 0;
> +    return SDVTInfo;
> +  }
> +
> +  unsigned getHashValue() const;
> +  bool operator==(const SDVTListInfo &SDVTInfo) const;
> +  bool operator!=(const SDVTListInfo &SDVTInfo) const { return !(*this 
> +== SDVTInfo); } };
> +
> +template <>
> +struct DenseMapInfo<SDVTListInfo> {
> +  static SDVTListInfo getEmptyKey() { return 
> +SDVTListInfo::getEmptyKey(); }
> +  static SDVTListInfo getTombstoneKey() { return 
> +SDVTListInfo::getTombstoneKey(); }
> +  static bool isEqual(SDVTListInfo LHS, SDVTListInfo RHS) { return LHS 
> +== RHS; }
> +  static unsigned getHashValue(const SDVTListInfo &Key) { return 
> +Key.getHashValue(); } };
> +
> /// SDDbgInfo - Keeps track of dbg_value information through SDISel.  We do  /// not build SDNodes for these so as not to perturb the generated code;  /// instead the info is kept off to the side in this structure. Each SDNode may @@ -1093,7 +1129,7 @@
>   void allnodes_clear();
> 
>   /// VTList - List of non-single value types.
> -  std::vector<SDVTList> VTList;
> +  SmallDenseMap<SDVTListInfo, SDVTList, 256> VTListMap;
> 
>   /// CondCodeNodes - Maps to auto-CSE operations.
>   std::vector<CondCodeSDNode*> CondCodeNodes;
> Index: lib/CodeGen/SelectionDAG/SelectionDAG.cpp
> ===================================================================
> --- lib/CodeGen/SelectionDAG/SelectionDAG.cpp
> +++ lib/CodeGen/SelectionDAG/SelectionDAG.cpp
> @@ -52,6 +52,23 @@
> #include <cmath>
> using namespace llvm;
> 
> +unsigned SDVTListInfo::getHashValue() const {
> +  unsigned hash = 0;
> +  for (unsigned index = 0; index < NumVTs; ++index) {
> +    hash = hash ^ pVTs[index].getRawBits();
> +  }
> +  hash = (hash << 2) ^ NumVTs;
> +  return hash;
> +}
> +
> +bool SDVTListInfo::operator==(const SDVTListInfo &SDVTInfo) const {
> +  if (NumVTs != SDVTInfo.NumVTs)
> +    return false;
> +  if (!std::equal(&pVTs[0], &pVTs[NumVTs], &SDVTInfo.pVTs[0]))
> +    return false;
> +  return true;
> +}
> +
> /// makeVTList - Return an instance of the SDVTList struct initialized with the  /// specified members.
> static SDVTList makeVTList(const EVT *VTs, unsigned NumVTs) { @@ -4891,75 +4908,31 @@  }
> 
> SDVTList SelectionDAG::getVTList(EVT VT1, EVT VT2) {
> -  for (std::vector<SDVTList>::reverse_iterator I = VTList.rbegin(),
> -       E = VTList.rend(); I != E; ++I)
> -    if (I->NumVTs == 2 && I->VTs[0] == VT1 && I->VTs[1] == VT2)
> -      return *I;
> -
> -  EVT *Array = Allocator.Allocate<EVT>(2);
> -  Array[0] = VT1;
> -  Array[1] = VT2;
> -  SDVTList Result = makeVTList(Array, 2);
> -  VTList.push_back(Result);
> -  return Result;
> +  const EVT pVTs[2] = {VT1, VT2};
> +  return getVTList(pVTs, 2);
> }
> 
> SDVTList SelectionDAG::getVTList(EVT VT1, EVT VT2, EVT VT3) {
> -  for (std::vector<SDVTList>::reverse_iterator I = VTList.rbegin(),
> -       E = VTList.rend(); I != E; ++I)
> -    if (I->NumVTs == 3 && I->VTs[0] == VT1 && I->VTs[1] == VT2 &&
> -                          I->VTs[2] == VT3)
> -      return *I;
> -
> -  EVT *Array = Allocator.Allocate<EVT>(3);
> -  Array[0] = VT1;
> -  Array[1] = VT2;
> -  Array[2] = VT3;
> -  SDVTList Result = makeVTList(Array, 3);
> -  VTList.push_back(Result);
> -  return Result;
> +  const EVT pVTs[3] = {VT1, VT2, VT3};
> +  return getVTList(pVTs, 3);
> }
> 
> SDVTList SelectionDAG::getVTList(EVT VT1, EVT VT2, EVT VT3, EVT VT4) {
> -  for (std::vector<SDVTList>::reverse_iterator I = VTList.rbegin(),
> -       E = VTList.rend(); I != E; ++I)
> -    if (I->NumVTs == 4 && I->VTs[0] == VT1 && I->VTs[1] == VT2 &&
> -                          I->VTs[2] == VT3 && I->VTs[3] == VT4)
> -      return *I;
> -
> -  EVT *Array = Allocator.Allocate<EVT>(4);
> -  Array[0] = VT1;
> -  Array[1] = VT2;
> -  Array[2] = VT3;
> -  Array[3] = VT4;
> -  SDVTList Result = makeVTList(Array, 4);
> -  VTList.push_back(Result);
> -  return Result;
> +  const EVT pVTs[4] = {VT1, VT2, VT3, VT4};  return getVTList(pVTs, 4);
> }
> 
> SDVTList SelectionDAG::getVTList(const EVT *VTs, unsigned NumVTs) {
> -  switch (NumVTs) {
> -    case 0: llvm_unreachable("Cannot have nodes without results!");
> -    case 1: return getVTList(VTs[0]);
> -    case 2: return getVTList(VTs[0], VTs[1]);
> -    case 3: return getVTList(VTs[0], VTs[1], VTs[2]);
> -    case 4: return getVTList(VTs[0], VTs[1], VTs[2], VTs[3]);
> -    default: break;
> -  }
> -
> -  for (std::vector<SDVTList>::reverse_iterator I = VTList.rbegin(),
> -       E = VTList.rend(); I != E; ++I) {
> -    if (I->NumVTs != NumVTs || VTs[0] != I->VTs[0] || VTs[1] != I->VTs[1])
> -      continue;
> -
> -    if (std::equal(&VTs[2], &VTs[NumVTs], &I->VTs[2]))
> -      return *I;
> -  }
> -
> +  SDVTListInfo Key = {VTs, NumVTs};
> +  if (VTListMap.count(Key))
> +    return VTListMap[Key];
> +  
>   EVT *Array = Allocator.Allocate<EVT>(NumVTs);
>   std::copy(VTs, VTs+NumVTs, Array);
>   SDVTList Result = makeVTList(Array, NumVTs);
> -  VTList.push_back(Result);
> +  Key.pVTs = Array;
> +  VTListMap.insert(std::make_pair(Key, Result));
> +
>   return Result;
> }
> <D1127.3.patch>