[PATCH] Global merge on constants

[Quentin Colombet]

Hi Bill,

I’ve attached the measurement made on compile time for O3.
In that file, the Reference column is llvm r177120 without the proposed patch and Test is  the same compiler with the proposed patch and constant merging enabled.
Both compilers have been generated without assertions and with optimizations (i.e., release mode).

The speedup column reports a ratio: Test/Reference, thus the bigger the better.

On average, the impact is limited (0.99 speedup, thus 1% slow-down). The maximum and minimum reported numbers are, in my opinion, not much relevant as the related tests (SingleSource/UnitTests/Vector/build and MultiSource/Benchmarks/MiBench/network-dijkstra/network-dijkstr) are compiled very quickly and the measurement method is not that accurate.
Moreover, we have to keep in mind that this patch adds a check for a currently bogus behavior with global variables maked as “used”.

Feel free to comment!

Thanks,

-Quentin

On Mar 14, 2013, at 5:03 PM, Quentin Colombet <qcolombet at apple.com> wrote:

> Thanks again Bill.
> 
> I’m running the test suite at O3 and I’ll attached the compile time results with the updated patch.
> 
> On Mar 14, 2013, at 4:58 PM, Bill Wendling <wendling at apple.com> wrote:
> 
>> On Mar 14, 2013, at 4:53 PM, Quentin Colombet <qcolombet at apple.com> wrote:
>> 
>>> Thanks Bill, I appreciate even if it’s "a bit self-serving" :).
>>> 
>>> On Mar 14, 2013, at 4:24 PM, Bill Wendling <wendling at apple.com> wrote:
>>> 
>>>> On Mar 14, 2013, at 4:02 PM, Quentin Colombet <qcolombet at apple.com> wrote:
>>>> 
>>>>> Hi,
>>>>> 
>>>>> I’ve updated the patch for Global merge constants so that it implements the last proposal.
>>>>> 
>>>>> Thanks for your reviews,
>>>>> 
>>>> It's a bit self-serving for me to review this, given that I suggested it, but here we go. :-) Overall, this looks good. I would be interested to know how this affects compile time --- specifically at '-O0'. I ask because of looping over the uses of a global in a program can take a while for something that's used a lot. Could you run the test-suite to see if there are any significant slowdowns?
>>> Will do. However, this pass is obviously not run at O0.
>>> 
>> Ah! yes. I had a brain crash. :)
>> 
>>>> 
>>>> There are a few nits:
>>>> 
>>>> +/// Check whether the uses of the given value can be replaced by GEP.
>>>> 
>>>> Should this start with a '\brief’?
>>> Good question…
>>> doxygen can interpret the first line of such comments as a brief, but it depends on how it is configured so I don’t know.
>>> Anyway, it does not hurt to add one in that case :).
>>> 
>>>> +/// This function also checks transitively the uses made after a bitcast of the
>>>> +/// given value.
>>>> +/// Currently we consider that this is not possible when at least one of the
>>>> +/// use is one of these kind:
>>>> +/// - Intrinsic
>>>> +/// - Landing pad
>>>> +/// - Inline asm
>>>> +/// \pre Val is a GlobalVariable or a bitcast of a GlobalVariable or a bitcast
>>>> +///      of a bitcast of a GlobalVariable and so on.
>>>> +static bool areUsesAcceptGEP(const Value *Val) {
>>>> 
>>>> Do you mean 'areUsesExceptGEP’?
>>> No, I want to check if all the uses can have a GEP as one of their operand, i.e., it won’t break the IR if we turn one of their operand into a GEP.
>> 
>> Ah! okay. That makes sense.
>> 
>>> Maybe doUsesAcceptGEP is more correct?
>>> If you have something else in mind, I’m sure it will be better!
>>> 
>> How about 'canBeReplacedByGEP’?
> Love it!
> Moreover, it’s consistent with the comment I wrote for the function :).
> Had a brain crash here too!
> 
>> 
>>>> +    if (CI && isa<const InlineAsm>(CI->getCalledValue()))
>>>> +      return false;
>>>> +    // Check transitively uses done after a bitcasts
>>>> +    const BitCastInst *BI = dyn_cast<const BitCastInst>(*UseIt);
>>>> +    // FIXME: Is it possible to have a cycle with bitcasts uses, i.e.,
>>>> +    // value1 bitcasted to value2 and value2 bitcasted to value1 without
>>>> +    // anything in between?
>>>> 
>>>> I don't think it's possible with SSA. I'm assuming you mean something like this?
>>> That’s what I think too: because of SSA and because you cannot change the address of a global variable.
>>> I can remove this comment then!
>>> 
>>> For the record, I had in mind something like this (which is syntactically wrong):
>>> @g = i32*
>>> @h = float*
>>> 
>>> define @foo() {
>>>  @h = bitcast i32* @g to float*
>>> }
>>> 
>>> define @bar() {
>>>  @g = bitcast float* @h to i32*
>>> }
>>> 
>> Yeah. I don't think you can do that. :)
>> 
>> -bw
> 
> _______________________________________________
> llvm-commits mailing list
> 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/20130315/5da56cc7/attachment.html>
-------------- next part --------------
An embedded and charset-unspecified text was scrubbed...
Name: compile_time.txt
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20130315/5da56cc7/attachment.txt>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20130315/5da56cc7/attachment-0001.html>