[PATCH] An intrinsic and lowering for instrumentation based profiling

[Duncan P. N. Exon Smith]

> On 2014 Dec 5, at 16:28, Justin Bogner <mail at justinbogner.com> wrote:
> 
> "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.
> 
> <instrprof-intrinsic.1.patch>

LGTM.