[PATCH] Global merge on constants

Quentin Colombet qcolombet at apple.com
Fri Mar 15 10:13:26 PDT 2013


The updated patch.

-Quentin


On Mar 15, 2013, at 9:51 AM, Quentin Colombet <qcolombet at apple.com> wrote:

> 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
> <compile_time.txt>
> 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
> 
> _______________________________________________
> 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/c16abe09/attachment.html>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: GlobalMergeOnConst.patch
Type: application/octet-stream
Size: 7269 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20130315/c16abe09/attachment.obj>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20130315/c16abe09/attachment-0001.html>


More information about the llvm-commits mailing list