[PATCH][Review Requested][Compilation Time] Small Vector for Machine Operand

Bill Wendling wendling at apple.com
Wed Feb 27 11:53:32 PST 2013 

Sorry. This fell off of my plate.

All of the changes from std::vector to SmallVector are obvious and can be committed.

I'm confused by why you changed the 'SUnits' ivar in ScheduleDAGTopologicalSort into a pointer rather than a reference. What did that gain you?

Andy is the one who should review the meat of this patch: creating the scheduler once instead of every time through the function. Is there state being left around from previous runs? If so, that needs to be cleared out each time.

-bw

On Feb 27, 2013, at 11:35 AM, "Nowicki, Tyler" <tyler.nowicki at intel.com> wrote:

> Hi,
> 
> Could someone take a look at the updated patch?
> 
> Thanks,
> 
> Tyler
> 
> -----Original Message-----
> From: Nowicki, Tyler 
> Sent: Thursday, February 14, 2013 5:10 PM
> To: Bill Wendling
> Cc: Jakub Staszak; llvm-commits at cs.uiuc.edu; Jakub Staszak
> Subject: RE: [PATCH][Review Requested][Compilation Time] Small Vector for Machine Operand
> 
> Hi BW,
> 
> Thanks for the review and the examples. I'm still learning LLVM so I found them quite helpful.
> 
> ArrayRef<> - This uses a const pointer to the array. Unfortunately much of LLVMs ScheduleDAG, and its various priority queues were not implemented with this in mind. I looked into changing the constness of the SUnits and SNodes, however, it looks like a lot of work. There is the added problem that when a SmallVector is modified the array may be destroyed. This would leave the ArrayRefs hanging. I found this to occur when I tried to use an ArrayRef for ScheduleDAGTopologicalSort.
> 
> SmallVectorImpl - I changed many of the SmallVectors to SmallVectorImpls.
> 
> Performance - The performance numbers in the previous email were a little confusing, but they did show an improvement. I ran them again on the updated patch and made the results easier to read (see below). They are measuring the time LLC takes to turn some bc files into machine code. The bc files were sampled from several benchmarks including SPEC.
> 
> '.reserve()' - Thanks yes we have several other patches some of which adjust reserve sizes, but std::vector is slower than SmallVector so we have mostly been changing types.
> 
> Tyler
> 
> Benchmark		% improvement
> 401.bzip2		1.54%
> 429.mcf		1.91%
> 433.milc		0.96%
> 444.namd		1.35%
> 450.soplex		2.32%
> 456.hmmer		1.85%
> 458.sjeng		1.75%
> 464.h264ref		1.28%
> 470.lbm		0.07%
> 471.omnetpp		2.31%
> bitmnp01		2.31%
> cjpegv2data6		1.41%
> idctrn01		0.92%
> libquake_portable	1.83%
> linpack			1.03%
> matrix01		1.32%
> nbench			1.31%
> tblook01		0.51%
> ttsprk01		0.88%
> 
> geomean		1.42%
> 
> -----Original Message-----
> From: Bill Wendling [mailto:wendling at apple.com] 
> Sent: Monday, February 04, 2013 4:11 PM
> To: Nowicki, Tyler
> Cc: Jakub Staszak; llvm-commits at cs.uiuc.edu; Jakub Staszak
> Subject: Re: [PATCH][Review Requested][Compilation Time] Small Vector for Machine Operand
> 
> Hi Tyler,
> 
> When passing around vectors that you're not going to modify, please use an 'ArrayRef<>' instead. The numbers for the SPEC tests seemed to increase. Is that a good thing? I.e., what are those numbers measuring? Have you considered using a '.reserve()' call on the std::vectors to limit the amount of reallocating and copying it does?
> 
> -bw
> 
> P.S. If you're going to pass a SmallVector around that you plan on modifying, you should use this:
> 
> 	void foo(SmallVectorImpl<SomeType> &fnord) {}
> 
> You can use the same when looping:
> 
> 	for (SmallVectorImpl<SomeType>::iterator I = ...)
> 
> That way you don't have to worry about the SmallVector count.
> 
> On Feb 4, 2013, at 12:22 PM, "Nowicki, Tyler" <tyler.nowicki at intel.com> wrote:
> 
>> Could someone please review this patch.
>> 
>> Thanks,
>> 
>> Tyler
>> 
>> From: llvm-commits-bounces at cs.uiuc.edu [mailto:llvm-commits-bounces at cs.uiuc.edu] On Behalf Of Nowicki, Tyler
>> Sent: Thursday, January 31, 2013 5:10 PM
>> To: Jakub Staszak
>> Cc: llvm-commits at cs.uiuc.edu; Jakub Staszak
>> Subject: RE: [PATCH][Review Requested][Compilation Time] Small Vector for Machine Operand
>> 
>> Oops, thanks.
>> 
>> Here is the patch...
>> 
>> Tyler
>> 
>> From: Jakub Staszak [mailto:kubastaszak at gmail.com] 
>> Sent: Thursday, January 31, 2013 5:01 PM
>> To: Nowicki, Tyler
>> Cc: Jakub Staszak; llvm-commits at cs.uiuc.edu
>> Subject: Re: [PATCH][Review Requested][Compilation Time] Small Vector for Machine Operand
>> 
>> Looks like you forgot to attach the patch :)
>> 
>> - Kuba
>> 
>> On Jan 31, 2013, at 7:58 PM, "Nowicki, Tyler" <tyler.nowicki at intel.com> wrote:
>> 
>> 
>> This patch aims to improve compile time performance by changing when the SchedulerDAG object is created and destroyed, and replacing std::vector with SmallVector. Currently, ScheduleDAG is deleted after each BB which causes frequent SUnits vector clearance, and std::vector causes frequent element copies the vector size grows. Please see attached performance data.
>> 
>> This patch is part of a series of compile time improvements. Although these were originally produced by our colleague Wan Xiaofei, our team consisting of preston.gurd at intel.com; sriram.murali at intel.com and myself, are assuming all responsibility for this work.
>> 
>> PLEASE REVIEW. Thanks!
>> 
>> Tyler Nowicki
>> Intel
>> 
>> Benchmark
>> Trunk
>> SmallVec.SU
>> 401.bzip2
>> 74.21
>> 72.48
>> 403.gcc
>> 73.88
>> 71.65
>> 429.mcf
>> 72.80
>> 71.33
>> 433.milc
>> 78.78
>> 78.14
>> 444.namd
>> 94.73
>> 93.45
>> 445.gobmk
>> 36.28
>> 35.51
>> 450.soplex
>> 71.41
>> 69.64
>> 456.hmmer
>> 86.80
>> 84.86
>> 458.sjeng
>> 96.38
>> 94.27
>> 464.h264ref
>> 87.61
>> 86.36
>> 470.lbm
>> 68.95
>> 68.09
>> 471.omnetpp
>> 89.07
>> 86.57
>> bitmnp01
>> 84.06
>> 81.60
>> cjpegv2data6
>> 59.70
>> 58.84
>> idctrn01
>> 40.18
>> 39.79
>> libquake2
>> 48.48
>> 47.54
>> libquake_portable
>> 63.54
>> 62.38
>> libxcsoar
>> 47.44
>> 46.81
>> linpack
>> 142.14
>> 141.63
>> matrix01
>> 24.75
>> 24.40
>> nbench
>> 108.04
>> 107.04
>> tblook01
>> 44.03
>> 43.71
>> ttsprk01
>> 39.23
>> 38.54
>> Geomean
>> 65.85
>> 64.72
>> 401.bzip2
>> 100.00
>> 102.39
>> 403.gcc
>> 100.00
>> 103.11
>> 429.mcf
>> 100.00
>> 102.06
>> 433.milc
>> 100.00
>> 100.82
>> 444.namd
>> 100.00
>> 101.37
>> 445.gobmk
>> 100.00
>> 102.17
>> 450.soplex
>> 100.00
>> 102.54
>> 456.hmmer
>> 100.00
>> 102.29
>> 458.sjeng
>> 100.00
>> 102.24
>> 464.h264ref
>> 100.00
>> 101.45
>> 470.lbm
>> 100.00
>> 101.26
>> 471.omnetpp
>> 100.00
>> 102.89
>> bitmnp01
>> 100.00
>> 103.01
>> cjpegv2data6
>> 100.00
>> 101.46
>> idctrn01
>> 100.00
>> 100.98
>> libquake2
>> 100.00
>> 101.98
>> libquake_portable
>> 100.00
>> 101.86
>> libxcsoar
>> 100.00
>> 101.35
>> linpack
>> 100.00
>> 100.36
>> matrix01
>> 100.00
>> 101.43
>> nbench
>> 100.00
>> 100.93
>> tblook01
>> 100.00
>> 100.73
>> ttsprk01
>> 100.00
>> 101.79
>> Geomean
>> 100.00
>> 101.76
>> 
>> _______________________________________________
>> llvm-commits mailing list
>> llvm-commits at cs.uiuc.edu
>> http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits
>> 
>> _______________________________________________
>> llvm-commits mailing list
>> llvm-commits at cs.uiuc.edu
>> http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits
> 
> <SmallVector2-svn.patch>

More information about the llvm-commits mailing list