[PATCH] Global merge on constants

[Quentin Colombet]

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.

> 
> 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.
Maybe doUsesAcceptGEP is more correct?
If you have something else in mind, I’m sure it will be better!

> 
> +  for (Value::const_use_iterator UseIt = Val->use_begin(),
> +         EndIt = Val->use_end(); UseIt != EndIt; ++UseIt) {
> +    // Do not mess with Intrinsic or LandingPad
> +    if (isa<const LandingPadInst>(*UseIt) ||
> +        isa<const IntrinsicInst>(*UseIt))
> 
> You don't need the 'const' in the 'isa<>' statement.
Okay.

> 
> +      return false;
> +    // Same for InlineAsm
> +    const CallInst *CI = dyn_cast<const CallInst>(*UseIt); 
> 
> Same here, re 'const' in the 'dyn_cast<>' statement.
Okay.

> 
> +    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*
}

> 
> @g = i32* ...
> 
> define void @foo() {
>  %tmp1 = bitcast i32* @g to i8*
>  %tmp2 = bitcast i8* %tmp1 to i32*
> }
> 
> +    // If yes, we should keep a set of already seen values.
> +    if (BI && !areUsesAcceptGEP(BI))
> +      return false;
> +  }
> +  return true;
> +}
> 
> -bw

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