[PATCH] Adding a timing option for IR parsing

[Andrew Trick]

On Mar 22, 2013, at 1:18 PM, Eli Bendersky <eliben at google.com> wrote:

>>> On Thu, Mar 21, 2013 at 7:24 PM, Chandler Carruth <chandlerc at google.com> wrote:
>>> FYI, I reverted the patch in r177695 to fix our builds. We check that layering violations don't get any worse in our build system.
>>> 
>>> Can you explain what kind of build this breaks?
>>> 
>>> I understand the layering problem, but how do we still get to measure the time IR parsing took? Do you have plans to fix the IR Reader violation any time soon?
>> 
>> To answer your middle question. Just reuse -time-passes and expose the flag in LLVMContext.
>> 
>> On the last question, I would like to see an answer to Chandler's question about whether anyone cares if they now automatically link the IR parser. Maybe Chandler should re-ask on llvm-dev?
>> 
>> Chatted with Chris on IRC and he seemed happier with a new library. I'm adding that now.
>> 
>> It's called "IRReader" so we have IRReader/IRReader.h... if anyone really wants a different name, shout...
>> 
>> So should the flag go back to IRReader or to LLVMContext? I still think it makes sense to have it separate because we have a bunch of LLVM tools that read IR but don't run any "passes".
> 
> Back now to Nadav's original question. Why do we need a new command line flag? I don't see a good reason. If someone want to -time-passes, I think they'll be happy to get IR parsing time too. If the tool doesn't run formal passes, then it just shows parsing time and any other times the tool chooses to add under the TimePassesIsEnabled flag.
> 
> -Andy
> 
> So you're proposing to place TimePassesIsEnabled in LLVMContext.cpp, and expose it (as well as the timer group name which will have to be reused in both PassManager and IRReader) in LLVMContext.h ? 
> 
> As a matter of personal opinion, this just seems really ugly to me. If I'm misunderstanding, could you just propose a patch that does this in a way you find natural? It's really not much code involved here... Mainly deciding where to put it.

If you want to commit something temporary before the IRReader lib is created, you can probably do a very quick hack:

-  extern bool TimeIRParsingIsEnabled;
+ extern bool TimePassesIsEnabled;

And simply delete your definition of the new flag:

-bool llvm::TimeIRParsingIsEnabled = false;
-static cl::opt<bool,true>
-EnableTimeIRParsing("time-ir-parsing", cl::location(TimeIRParsingIsEnabled),
-                    cl::desc("Measure the time IR parsing takes"));


When the IRReader lib is created we can move all the function definitions from IRReader.h into IRReader.cpp and get rid of the shady declaration of TimePassesIsEnabled. Just have IRReader.cpp include the necessary IR header. Currently it's Pass.h. If you don't like that, you can move the TimePassesIsEnabled into a more common location like LLVMContextImpl.h. You can even make it a flag within LLVMContextImpl to avoid globals.

There are different levels of cleanup here. My only concern with this patch is that you don't create another command line flag!

-Andy


-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20130322/3ee18be5/attachment.html>