[PATCH] Global merge on constants

[Bill Wendling]

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?

There are a few nits:

+/// Check whether the uses of the given value can be replaced by GEP.

Should this start with a '\brief'?

+/// 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'?

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

+      return false;
+    // Same for InlineAsm
+    const CallInst *CI = dyn_cast<const CallInst>(*UseIt); 

Same here, re 'const' in the 'dyn_cast<>' statement.

+    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?

@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