[PATCH] Use auto profiler to set branch weights

[Diego Novillo]

Ping.

Thanks.  Diego.

On Wed, Nov 6, 2013 at 7:26 PM, Diego Novillo <dnovillo at google.com> wrote:
>
>   I've incorporated all your feedback from this review and other email.  PTAL.
>
>
> ================
> Comment at: lib/Transforms/Scalar/SampleProfile.cpp:475-476
> @@ +474,4 @@
> +  FunctionProfile &FProfile = Profiles[F->getName()];
> +  if (FProfile.BlockWeights.find(B) != FProfile.BlockWeights.end())
> +    return FProfile.BlockWeights[B];
> +
> ----------------
> Chandler Carruth wrote:
>> This does the lookup twice. Store the iterator and use that.
> Done (incidentally, I looked around and code patterns I found implement it the way I had it).  Seems like this makes the code less readable because of all the .first .second map referencing. But I'm OK either way.
>
> ================
> Comment at: lib/Transforms/Scalar/SampleProfile.cpp:517
> @@ +516,3 @@
> +    SmallVector<uint32_t, 4> Weights;
> +    for (unsigned I = 0; I < TI->getNumSuccessors(); ++I) {
> +      BasicBlock *Succ = TI->getSuccessor(I);
> ----------------
> Chandler Carruth wrote:
>> Cache the number of successors in a loop variable, calling it on every iteration is very expensive.
> Done.
>
> ================
> Comment at: lib/Transforms/Scalar/SampleProfile.cpp:523
> @@ +522,3 @@
> +
> +    llvm::MDBuilder MDB(TI->getContext());
> +    llvm::MDNode *WeightsNode = MDB.createBranchWeights(Weights);
> ----------------
> Chandler Carruth wrote:
>> Build this outside of the loop.
> Done.
>
> ================
> Comment at: lib/Transforms/Scalar/SampleProfile.cpp:524
> @@ +523,3 @@
> +    llvm::MDBuilder MDB(TI->getContext());
> +    llvm::MDNode *WeightsNode = MDB.createBranchWeights(Weights);
> +    TI->setMetadata(llvm::LLVMContext::MD_prof, WeightsNode);
> ----------------
> Chandler Carruth wrote:
>> No need for a variable, just pass it directly to setMetadata.
> Done.
>
> ================
> Comment at: lib/Transforms/Scalar/SampleProfile.cpp:485
> @@ +484,3 @@
> +  }
> +  FProfile.BlockWeights[B] = Weight;
> +  return Weight;
> ----------------
> Chandler Carruth wrote:
>> Even better, since you're going to compute the weight if it isn't there already, try inserting '0' into the map. If the insert fails, return the weight pointed to by the iterator. If the insert succeeds, update the weight pointed to by the iterator like you do here, and then return that. This does exactly one probe into the hash table.
> Done.
>
>
> http://llvm-reviews.chandlerc.com/D2058