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

[Wan, Xiaofei]

Hi, Nadav:

Yes, there is one extreme case quake, in which getVTList() consumes a lot of compilation time(~2%).

Actually, this patch was sent out by my colleague for review in this Feb(I didn't have commit access at that time) and it was approved by Chris and Bill Wendling .
http://lists.cs.uiuc.edu/pipermail/llvm-commits/Week-of-Mon-20130225/167075.html

BTW, several other patches aiming at improving compilation time were reviewed and approved but were not submitted due to some changes in our internal org; I will continue this jobs since I am the author of these patches.
This patch is just a very minor one and it has a very obvious defect in getVTList since it use linear search.

Thanks
Wan Xiaofei
From: Nadav Rotem [mailto:nrotem at apple.com]
Sent: Friday, October 18, 2013 1:10 PM
To: Wan, Xiaofei
Cc: Owen Anderson; llvm-commits at cs.uiuc.edu LLVM
Subject: Re: [PATCH] [Review Request] Improve getVTList() in SelectionDAG

Hi Wan,

Have you measured the performance impact of your change (compile time) ?

Thanks,
Nadav

On Oct 17, 2013, at 4:40 AM, Wan, Xiaofei <xiaofei.wan at intel.com<mailto:xiaofei.wan at intel.com>> wrote:


Ping again.

Owen, is it OK to submit?

Thanks
Wan Xiaofei
From: llvm-commits-bounces at cs.uiuc.edu<mailto:llvm-commits-bounces at cs.uiuc.edu> [mailto:llvm-commits-bounces at cs.uiuc.edu] On Behalf Of Wan, Xiaofei
Sent: Sunday, October 13, 2013 12:50 AM
To: Owen Anderson
Cc: llvm-commits at cs.uiuc.edu<mailto:llvm-commits at cs.uiuc.edu> LLVM
Subject: RE: [PATCH] [Review Request] Improve getVTList() in SelectionDAG

Owen:

FoldingSet can implement similar concept, thanks.
Patch updated http://llvm-reviews.chandlerc.com/D1127

BTW, FoldingSet is a good data structure, but it copies many contents when implementing hash and cause a big performance penalty since it is wildly used in SelectionDAG; meanwhile the hash value is calculated many times for same object. Our profiling data shows it consume about ~3-~5% of total time in llc; We may need to optimize it.

Thanks
Wan Xiaofei
From: Owen Anderson [mailto:resistor at mac.com]
Sent: Saturday, October 12, 2013 2:08 AM
To: Wan, Xiaofei
Cc: llvm-commits at cs.uiuc.edu<mailto:llvm-commits at cs.uiuc.edu> LLVM
Subject: Re: [PATCH] [Review Request] Improve getVTList() in SelectionDAG

OK, I can see that it works.  It seems like you're basically implementing the same idiom as the existing FoldingSet class (http://llvm.org/docs/ProgrammersManual.html#llvm-adt-foldingset-h).  Can you see if you could make use of that rather than rolling your own solution?

-Owen

On Oct 10, 2013, at 10:26 PM, Wan, Xiaofei <xiaofei.wan at intel.com<mailto:xiaofei.wan at intel.com>> wrote:

Owen:

Thanks for your comments.
Patch updated http://llvm-reviews.chandlerc.com/D1127

SDVTList SelectionDAG::getVTList(EVT VT1, EVT VT2) {
 const EVT tmpVTs[2] = {VT1, VT2};
 return getVTList(tmpVTs, 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.
[Xiaofei] Yes, tmpVTs is an array on the stack, but it is just used temporally to determine whether this SDVTList  exist in the DenseMap, it won't be referenced anymore after that.

SDVTList SelectionDAG::getVTList(const EVT *VTs, unsigned NumVTs) {
 SDVTListInfo Key(VTs, NumVTs);
 if (VTListMap.count(Key))
   return VTListMap[Key];

 EVT *Array = Allocator.Allocate<EVT>(NumVTs);
 std::copy(VTs, VTs + NumVTs, Array); -------------------------------------------------VTs is copied out to Array which will be stored in the DenseMap
 SDVTList Result = makeVTList(Array, NumVTs);
 Key.pVTs = Array; --------------------------------------------------VTs is not used any more.
 VTListMap.insert(std::make_pair(Key, Result));

 return Result;
}

Thanks
Wan Xiaofei

-----Original Message-----
From: Owen Anderson [mailto:resistor at mac.com]
Sent: Friday, October 11, 2013 1:30 AM
To: Wan, Xiaofei
Cc: llvm-commits at cs.uiuc.edu<mailto:llvm-commits at cs.uiuc.edu> LLVM
Subject: Re: [PATCH] [Review Request] Improve getVTList() in SelectionDAG

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

<D1127.5.patch>

_______________________________________________
llvm-commits mailing list
llvm-commits at cs.uiuc.edu<mailto:llvm-commits at cs.uiuc.edu>
http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20131018/58a50f59/attachment.html>