[PATCH] An intrinsic and lowering for instrumentation based profiling

[Justin Bogner]

"Duncan P. N. Exon Smith" <dexonsmith at apple.com> writes:
>> On 2014-Dec-04, at 15:50, Justin Bogner <mail at justinbogner.com> wrote:
>> 
>> This introduces the ``llvm.instrprof_increment`` intrinsic and the
>> ``-instrprof`` pass. These provide the infrastructure for writing
>> counters for profiling, as in clang's ``-fprofile-instr-generate``.

>> +'``llvm.instrprof_increment``' Intrinsic
>> +^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
>> +
>> +Syntax:
>> +"""""""
>> +
>> +::
>> +
>> +      declare void @llvm.instrprof_increment(i8* <name>, i32 <num-counters>,
>> +                                             i64 <hash>, i64 <counter-index>)
>
> The types seem off: you have an `i32` for the number of counters but an
> `i64` for the index into them.  I think `i32` makes sense for both.
>
> Bike-shed: I think it'd be clearer to reorder the arguments so the
> counter index and number of counters are adjacent.  I'd prefer either of:
>
>     (i8* <name>, i64 <hash>, i32 <counter-index>, i32 <num-counters>)
>     (i8* <name>, i64 <hash>, i32 <num-counters>,  i32 <counter-index>)

Yep, I'm not sure how I ended up with the types of num-counters and
index mismatched, but this signature is obviously better.

>> +bool InstrProfiling::runOnModule(Module &M) {
>> +  bool MadeChange = false;
>> +
>> +  this->M = &M;
>> +  RegionCounters.clear();
>> +  UsedVars.clear();
>> +
>> +  for (Function &F : M)
>> +    for (BasicBlock &BB : F)
>> +      for (auto I = BB.begin(), E = BB.end(); I != E;)
>> +        if (auto *Inc = dyn_cast<InstrProfIncrementInst>(I++)) {
>> +          lowerIncrement(Inc);
>> +          MadeChange = true;
>> +        }
>> +  MadeChange |= emitRegistration();
>> +  MadeChange |= emitRuntimeHook();
>> +  MadeChange |= emitUses();
>> +  MadeChange |= emitInitialization();
>> +  return MadeChange;
>
> Why isn't this the following?
>
>     if (!MadeChange)
>       return false;
>
>     emitRegistration();
>     emitRuntimeHook();
>     emitUses();
>     emitInitialization();
>     return true;
>
> In particular, should the pass emit anything if there are no counters?
> Why?
>
> Can you add a test for when the pass gets run but there aren't any
> counters?  (What you're testing for depends on the above...)

It ended up being equivalent - nothing would be emitted if no increments
were lowered anyway. I've cleaned it up as per your suggestion and added
a test.

>> +  GlobalVariable *LLVMUsed = M->getGlobalVariable("llvm.used");
>> +  std::vector<Constant*> MergedVars;
>> +  if (LLVMUsed) {
>> +    // Collect the existing members of llvm.used.
>> +    ConstantArray *Inits = cast<ConstantArray>(LLVMUsed->getInitializer());
>> +    for (unsigned I = 0, E = Inits->getNumOperands(); I != E; ++I)
>> +      MergedVars.push_back(Inits->getOperand(I));
>> +    LLVMUsed->eraseFromParent();
>> +  }
>> +
>> +  Type *i8PTy = Type::getInt8PtrTy(M->getContext());
>> +  // Add uses for our data.
>> +  for (auto *Value : UsedVars)
>> +    MergedVars.push_back(
>> +        ConstantExpr::getBitCast(cast<llvm::Constant>(Value), i8PTy));
>> +
>> +  // Recreate llvm.used.
>> +  ArrayType *ATy = ArrayType::get(i8PTy, MergedVars.size());
>> +  LLVMUsed = new llvm::GlobalVariable(
>> +      *M, ATy, false, llvm::GlobalValue::AppendingLinkage,
>> +      llvm::ConstantArray::get(ATy, MergedVars), "llvm.used");
>> +
>> +  LLVMUsed->setSection("llvm.metadata");
>
> It feels kind of awkward to delete and recreate `@llvm.used`, I'd rather
> just reset its initializer.  But since you need to collect its current
> members anyway, maybe that's just as awkward...

I sketched it out and it's at least as awkward to replace the
initializer, so I've just left this as is.

New patch attached.

-------------- next part --------------
A non-text attachment was scrubbed...
Name: instrprof-intrinsic.1.patch
Type: text/x-patch
Size: 24564 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20141205/7b0952b4/attachment.bin>