[llvm-commits] [llvm] r150794 - in /llvm/trunk: lib/Transforms/IPO/GlobalOpt.cpp test/Transforms/GlobalOpt/invariant.ll

[Chris Lattner]

On Feb 16, 2012, at 10:59 PM, Nick Lewycky wrote:
> URL: http://llvm.org/viewvc/llvm-project?rev=150794&view=rev
> Log:
> Add support for invariant.start inside the static constructor evaluator. This is
> useful to represent a variable that is const in the source but can't be constant
> in the IR because of a non-trivial constructor. If globalopt evaluates the
> constructor, and there was an invariant.start with no matching invariant.end
> possible, it will mark the global constant afterwards.

Very cool optimization.  Some questions though:

> +++ llvm/trunk/lib/Transforms/IPO/GlobalOpt.cpp Fri Feb 17 00:59:21 2012
> @@ -2295,6 +2295,7 @@
>                              DenseMap<Constant*, Constant*> &MutatedMemory,
>                              std::vector<GlobalVariable*> &AllocaTmps,
>                              SmallPtrSet<Constant*, 8> &SimpleConstants,
> +                             SmallPtrSet<GlobalVariable*, 8> &Invariants,

This really needs to refactoring we were just talking about.  This is getting ridiculous :)

> @@ -2415,14 +2417,39 @@
> +        if (II->getIntrinsicID() == Intrinsic::invariant_start) {
> +          // We don't insert an entry into Values, as it doesn't have a
> +          // meaningful return value.
> +          if (!II->use_empty())
> +            return false;
> +          ConstantInt *Size = cast<ConstantInt>(II->getArgOperand(0));
> +          if (Size->isAllOnesValue()) {
> +            Value *PtrArg = getVal(Values, II->getArgOperand(1));
> +            Value *Ptr = PtrArg->stripPointerCasts();
> +            if (GlobalVariable *GV = dyn_cast<GlobalVariable>(Ptr))
> +              Invariants.insert(GV);
> +          }

Why are you checking for (and why is Richard generating) -1 for the object size here, instead of the proper size of the value?

> 
> +  %B = call {}* @llvm.invariant.start(i64 -1, i8* %A)
> +  ; Why in the world does this pass the verifier?
> +  %C = bitcast {}* %B to i8*

This is a bitcast like any other.   I'm not sure what the question is, but it shouldn't be in the testcase.


Finally, most significantly, it isn't clear to me how the semantics of invariant.start/end work for staticly constructed objects.  Per lang-ref, the return value of ".start" is supposed to be passed into ".end".  Given the usual clang codegen for static constructors - which uses an atexit handler to run the dtor - I don't see how we can possibly generate this.

I understand that Clang (just now) only generates .start when there is no destructor, but I want to make sure that the semantics of these intrinsics are fully nailed down in langref before we start building things on them.  In this case, it might be best to introduce a new intrinsic here instead of [ab]using .begin in a case when .end isn't possible.

Also, as requested on cfe-commits, please only generate optimization hints when the optimizer is enabled to avoid penalizing -O0 compile time.

Finally, we miss cases with explicit-but-trivial destructors, which is a pretty common pattern in some codebases.  Is there any way to generalize this optimization?  It seems worthwhile to special case this sort of thing in clang, but it would be even better for the optimizer to be able to handle it when it (successfully) removes atexit call.

struct foo {
  int x;
  foo(int x) : x(x) {}
  ~foo() { }
  };

const foo a(4);

const void *p = &a;

-Chris