LLVM Bugzilla is read-only and represents the historical archive of all LLVM issues filled before November 26, 2021. Use github to submit LLVM bugs

Bug 32352 - Provide a way for OptimizationRemarkEmitter::allowExtraAnalysis to check if (specific) remarks are enabled
Summary: Provide a way for OptimizationRemarkEmitter::allowExtraAnalysis to check if (...
Status: RESOLVED FIXED
Alias: None
Product: tools
Classification: Unclassified
Component: opt-viewer (show other bugs)
Version: trunk
Hardware: PC All
: P enhancement
Assignee: Unassigned LLVM Bugs
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2017-03-20 09:06 PDT by Adam Nemet
Modified: 2018-11-07 00:22 PST (History)
9 users (show)

See Also:
Fixed By Commit(s):


Attachments

Note You need to log in before you can comment on or make changes to this bug.
Description Adam Nemet 2017-03-20 09:06:59 PDT
There is already a need for this.  For example WholeProgramDevirt instantiates a remark just to check its Enabled field to see if remarks are on or not.  This actually does not work because clang does not use the Enabled field only (opt's -pass-remarks* do).

Also GlobalISel wants to do something like this and it's currently using a cl::opt as a work-around.
Comment 1 Adam Nemet 2017-05-08 08:29:22 PDT
Moving the discussion from llvm-dev here.  This was Vivek's question.

Hi Adam,

I looked into code related to above feature request and perhaps I am not yet clear about this. 
I have tried out following things:

1) allowExtraAnalysis function will have a string parameter which is pass name.

2) Find an appropriate entry point in Clang where we can make a shared_ptr for 
OptimizationRemarkPattern, OptimizationRemarkAnalysisPattern in LLVMContext class.

3) Use above changes so that allowExtraAnalysis can be used to check if specific
remark is on or not.

However I am confused because there is one more class DiagnosticInfo in LLVM which handles same thing for opt's flags, but why those informations are not used in allowExtraAnalysis() ? Have I understood the purpose of allowExtraAnalysis or not ?

Please guide here.

Sincerely,
Vivek
Comment 2 Adam Nemet 2017-05-08 10:01:27 PDT
Thanks for taking this on!

(In reply to Vivek from comment #1)
> 1) allowExtraAnalysis function will have a string parameter which is pass
> name.

I think it will also need the remark kind (e.g. DK_OptRemarkAnalysis) unless we want to return all three in a structure.  The latter may be useful since passes may want to cache these together in order to have zero-overhead checking to see whether the remarks are off.

> 
> 2) Find an appropriate entry point in Clang where we can make a shared_ptr
> for 
> OptimizationRemarkPattern, OptimizationRemarkAnalysisPattern in LLVMContext
> class.

I am assuming you were thinking of doing this to call isEnabled on the remark; unfortunately that does not work.  (I know that is what WholeProgramDevirt does too.) isEnabled is *not* used in clang in llvm (opt/llc):

https://github.com/llvm-mirror/clang/blob/master/lib/CodeGen/CodeGenAction.cpp#L598

I think that the new design should move isEnabled to the DiagnosticHandler in the context.  DiagnosticHandler is only a callback now so it should probably become a class.  Then clients can ask whether a kind of remark (OptRemark, OptRemarkMissed, OptRemarkAnalysis) is enabled for a pass specified by the pass name.

> 3) Use above changes so that allowExtraAnalysis can be used to check if
> specific
> remark is on or not.

Yes.

> However I am confused because there is one more` class DiagnosticInfo in LLVM
> which handles same thing for opt's flags, but why those informations are not
> used in allowExtraAnalysis() ? Have I understood the purpose of
> allowExtraAnalysis or not ?

As explained above.
Comment 3 Vivek Pandya 2017-05-09 04:45:19 PDT
(In reply to Adam Nemet from comment #2)
> Thanks for taking this on!
> 
> (In reply to Vivek from comment #1)
> > 1) allowExtraAnalysis function will have a string parameter which is pass
> > name.
> 
> I think it will also need the remark kind (e.g. DK_OptRemarkAnalysis) unless
> we want to return all three in a structure.  The latter may be useful since
> passes may want to cache these together in order to have zero-overhead
> checking to see whether the remarks are off.
> 
> > 
> > 2) Find an appropriate entry point in Clang where we can make a shared_ptr
> > for 
> > OptimizationRemarkPattern, OptimizationRemarkAnalysisPattern in LLVMContext
> > class.
> 
> I am assuming you were thinking of doing this to call isEnabled on the
> remark; unfortunately that does not work.  (I know that is what
> WholeProgramDevirt does too.) isEnabled is *not* used in clang in llvm
> (opt/llc):
> 
> https://github.com/llvm-mirror/clang/blob/master/lib/CodeGen/CodeGenAction.
> cpp#L598
No actually I wanted to keep pointer for CodeGenOpts.OptimizationRemarkPattern and other similar objects in LLVMContext so that we can take decision based on clang's -Rpass options.
With that thought I am confused because as opt/llc and clang uses different options for same thing and user passes both things (particularly in clang though -mllvm) then it can creates problem. So I thought we can teach clang that drop opt/llc's options for ORE.

-Vivek
> 
> I think that the new design should move isEnabled to the DiagnosticHandler
> in the context.  DiagnosticHandler is only a callback now so it should
> probably become a class.  Then clients can ask whether a kind of remark
> (OptRemark, OptRemarkMissed, OptRemarkAnalysis) is enabled for a pass
> specified by the pass name.
> 
> > 3) Use above changes so that allowExtraAnalysis can be used to check if
> > specific
> > remark is on or not.
> 
> Yes.
> 
> > However I am confused because there is one more` class DiagnosticInfo in LLVM
> > which handles same thing for opt's flags, but why those informations are not
> > used in allowExtraAnalysis() ? Have I understood the purpose of
> > allowExtraAnalysis or not ?
> 
> As explained above.
Comment 4 Vivek Pandya 2017-05-09 05:04:13 PDT
(In reply to Vivek Pandya from comment #3)
> (In reply to Adam Nemet from comment #2)
> > Thanks for taking this on!
> > 
> > (In reply to Vivek from comment #1)
> > > 1) allowExtraAnalysis function will have a string parameter which is pass
> > > name.
> > 
> > I think it will also need the remark kind (e.g. DK_OptRemarkAnalysis) unless
> > we want to return all three in a structure.  The latter may be useful since
> > passes may want to cache these together in order to have zero-overhead
> > checking to see whether the remarks are off.
> > 
> > > 
> > > 2) Find an appropriate entry point in Clang where we can make a shared_ptr
> > > for 
> > > OptimizationRemarkPattern, OptimizationRemarkAnalysisPattern in LLVMContext
> > > class.
> > 
> > I am assuming you were thinking of doing this to call isEnabled on the
> > remark; unfortunately that does not work.  (I know that is what
> > WholeProgramDevirt does too.) isEnabled is *not* used in clang in llvm
> > (opt/llc):
> > 
> > https://github.com/llvm-mirror/clang/blob/master/lib/CodeGen/CodeGenAction.
> > cpp#L598
> No actually I wanted to keep pointer for
> CodeGenOpts.OptimizationRemarkPattern and other similar objects in
> LLVMContext so that we can take decision based on clang's -Rpass options.
> With that thought I am confused because as opt/llc and clang uses different
> options for same thing and user passes both things (particularly in clang
> though -mllvm) then it can creates problem. So I thought we can teach clang
> that drop opt/llc's options for ORE.
I am not sure why currently it is not done but when -fsave-optimization-record is present then clang should covert -Rpass to -mllvm equivalent opt/llc's option. 
> 
> -Vivek
> > 
> > I think that the new design should move isEnabled to the DiagnosticHandler
> > in the context.  DiagnosticHandler is only a callback now so it should
> > probably become a class.  Then clients can ask whether a kind of remark
> > (OptRemark, OptRemarkMissed, OptRemarkAnalysis) is enabled for a pass
> > specified by the pass name.
> > 
> > > 3) Use above changes so that allowExtraAnalysis can be used to check if
> > > specific
> > > remark is on or not.
> > 
> > Yes.
> > 
> > > However I am confused because there is one more` class DiagnosticInfo in LLVM
> > > which handles same thing for opt's flags, but why those informations are not
> > > used in allowExtraAnalysis() ? Have I understood the purpose of
> > > allowExtraAnalysis or not ?
> > 
> > As explained above.
Comment 5 Hal Finkel 2017-05-09 09:19:09 PDT
> I am not sure why currently it is not done but when -fsave-optimization-record is present then clang should covert -Rpass to -mllvm equivalent opt/llc's option. 

-mllvm options are for compiler development purposes only; they're not part of the user interface. Moreover, using command-line options to pass information between the frontend and the backend is highly discouraged.
Comment 6 Vivek Pandya 2017-08-16 22:56:43 PDT
Work in progress patch is https://reviews.llvm.org/D33514
Comment 7 Adam Nemet 2017-09-19 16:46:01 PDT
Vivek fixed this in r313382.  Thanks Vivek!