[llvm-commits] [llvm] r82062 - /llvm/trunk/include/llvm/Support/IRBuilder.h

[Devang Patel]

Chris,


On Sun, Sep 27, 2009 at 3:35 PM, Chris Lattner <clattner at apple.com> wrote:
> On Sep 16, 2009, at 11:16 AM, Devang Patel wrote:
>> URL: http://llvm.org/viewvc/llvm-project?rev=82062&view=rev
>> Log:
>> Add an interface to attach debugging information with an instruction.
>
> Ok, a couple of quibbles:
>
>> +++ llvm/trunk/include/llvm/Support/IRBuilder.h Wed Sep 16 13:16:11
>> 2009
>> @@ -20,6 +20,7 @@
>> #include "llvm/GlobalAlias.h"
>> #include "llvm/GlobalVariable.h"
>> #include "llvm/Function.h"
>> +#include "llvm/Metadata.h"
>
> This shouldn't be needed with changes below:
>
>> #include "llvm/LLVMContext.h"
>> #include "llvm/ADT/Twine.h"
>> #include "llvm/Support/ConstantFolder.h"
>> @@ -60,35 +61,38 @@
>> class IRBuilder : public Inserter {
>>   BasicBlock *BB;
>>   BasicBlock::iterator InsertPt;
>> +  MDKindID MDKind;
>> +  MDNode *CurLocation;
>>   LLVMContext &Context;
>>   T Folder;
>
> ...
>
>>
>> +  /// SetCurrentLocation - This specifies the location information
>> used
>> +  /// by debugging information.
>> +  void SetCurrentLocation(MDNode *L) {
>> +    if (MDKind == 0) {
>
> MDKind should be an argument to SetCurrentLocation.  IRBuilder should
> not implicitly look it up.

IMO IRBuilder clients do not need to know about MDKind that is used
for debug info. In fact they don't know how debug location info is
encoded. The IRBuilder handles this transparently. The FE keeps track
of current source location for many reasons anyway. Whenever the
location is changed, the IRBuilder is notified (through
SetCurrentDebugLocation) if -g is specified on the command line. And
IRBuilder is responsible to encode this appropriately in IR. This is
why I renamed it as "SetCurrentDebugLocation().

>
> Also, you renamed SetCurrentLocation -> SetCurrentDebugLocation.  Why
> not rename it to "SetDebugLocation"?

-
Devang

>
>>   /// Insert - Insert and return the specified instruction.
>>   template<typename InstTy>
>>   InstTy *Insert(InstTy *I, const Twine &Name = "") const {
>>     this->InsertHelper(I, Name, BB, InsertPt);
>> +    if (CurLocation)
>> +      Context.getMetadata().setMD(MDKind, CurLocation, I);
>
> We should have an API on instruction that forwards to the MD api:
>    Something like: I->AddMD(MDKind, CurLocation);
>
> Most clients should only poke the "Metadata" class when they register
> their MDKind, and they should pass them around as opaque unsigned's.
>
> -Chris
>
> _______________________________________________
> llvm-commits mailing list
> llvm-commits at cs.uiuc.edu
> http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits
>