[PATCH] Global merge on constants

[Quentin Colombet]

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

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