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

Nowicki, Tyler tyler.nowicki at intel.com
Tue Mar 12 11:38:34 PDT 2013


Sorry for the delayed response.

Thanks for the great review. I didn't realize that using SmallVector may have unforeseen consequence for other backends, just seemed like a move to an LLVM data type.

I am interested in learning more about your future changes to SelectionDAG. How will you resolve physical registers, and are you thinking of getting rid of preRA scheduling?

I'm not sure how we would verify that the preRA scheduler behavior hasn't changed over all backends. Do you have any suggestions?

Tyler

-----Original Message-----
From: Andrew Trick [mailto:atrick at apple.com] 
Sent: Wednesday, February 27, 2013 10:10 PM
To: Bill Wendling
Cc: Nowicki, Tyler; Jakub Staszak; llvm-commits at cs.uiuc.edu
Subject: Re: [PATCH][Review Requested][Compilation Time] Small Vector for Machine Operand


On Feb 27, 2013, at 11:53 AM, Bill Wendling <wendling at apple.com> wrote:

> 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.

Tyler,

This is good work, and I appreciate people chipping away at inneffeciencies. But I'm not sure this patch is worthwhile at this time.

Reusing the scheduler object is a no-brainer. That's how the MachineScheduler pass works. It's risky to do it for ScheduleDAGSDNodes because it's hard to be sure the subclasses are properly reinitializing themselves.

I suspect that when the SUnits vector is reused, converting it to a (very large) SmallVector doesn't help. I'd prefer not to change the underlying data type used by all schedulers (not just ScheduleDAGSDNodes) unless we know it's generally helpful.

The way to fix this is to start disabling ScheduleDAGSDNodes for targets. That will give us a much better speedup. Once all targets are weaned from ScheduleDAGSD, we can undertake a major streamlining of the common data structures. This is currently blocked because we don't have a way to resolve physical register directly on the SelectionDAG, but it will start happening in the next couple months.

I would be ok taking this patch in the very short term only if we knew the preRA scheduler behavior doesn't change as a result of reusing the scheduler object. I can't easily verify that. Can you guarantee it?

-Andy

> 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