Page MenuHomePhabricator

[WIP] Bug 32352 - Provide a way for OptimizationRemarkEmitter::allowExtraAnalysis to check if (specific) remarks are enabled
AcceptedPublic

Authored by vivekvpandya on May 24 2017, 12:05 PM.

Details

Reviewers
anemet
hfinkel
Summary

This patch first attempt to move isEnable() from DiagnosticInfo class to new class called DiagnosticHandler. I have still not moved emit() methods so I have still kept the call back. I am still having 8 lit test failing for this patch but I will solve the issues also my repo is not updated with upstream. I will fix all these issues and remaining tasks in upcoming updates. There is also very small change in clang's lib/CodeGen/CodeGenAction.cpp to assign clang's -Rpass option related shared pointers in DiagnosticHandler.
The motivation for this patch is to get early feedback so that I don't go into wrong direction.
Please provide feedback. This change also effects clang so I have added cfe-commits in subscriber .

Diff Detail

Event Timeline

There are a very large number of changes, so older changes are hidden. Show Older Changes
anemet added inline comments.Jul 27 2017, 2:43 PM
include/llvm/IR/DiagnosticHandler.h
29

I believe that this needs to be virtual too but it depends on the mechanism of how ...

30–32

I am assuming you expect to use these from clang (currently unused)? I don't think that's how we want to do it. Clang should simply derive from this class and override handleDiagnostics and is*RemarkEnable according to its own logic.

33

handleDiagnostics

34–36

I think that these need to be virtual member functions. See previous comment about the clang interface.

tools/lto/lto.cpp
281–282

Use make_unique

vivekvpandya added inline comments.Jul 31 2017, 9:54 AM
include/llvm/IR/DiagnosticHandler.h
26–28

This is required for LLVM-C API users. We have discussed this in earlier emails.

28

I don't get the benefit of returning combination of the flags. What I understood is that remark can be for Analysis pass , applied/passed optimization or missed optimization. So that is why I kept four values for enum, three of them is as per above category and one for user not requested remarks for particular reg-ex.

30–32

Yes what you have think is correct about this. Also what you have suggested is better, but I kept it this way, the reason is in next comment.

34–36

I want there method to be static because -Rpass options are static cl::opt instance so if we want to just check if user has requested particular diagnostics through command line then it should be doable without passing object or pointer of DiagnosticHandler class. I feel we can make isRemadkEnable(...) a virtual method but there methods should be kept static. But these method to work correctly we need shared_ptr to RegEx (in clang's case too)

anemet added inline comments.Aug 7 2017, 10:11 PM
include/llvm/IR/DiagnosticHandler.h
34–36

This is what I am proposing:

include/IR/DiagnosticHandler.h:

class DiagnosticHandler {
public:
  virtual bool isAnalysisRemarkEnabled(StringRef PassName);
   ...
};

lib/IR/DiagnosticInfo.cpp:

static PassRemarksOpt PassRemarksOptLoc;

static cl::opt<PassRemarksOpt, true, cl::parser<std::string>>
PassRemarks("pass-remarks", cl::value_desc("pattern"),
            cl::desc("Enable optimization remarks from passes whose name match "
                     "the given regular expression"),
            cl::Hidden, cl::location(PassRemarksOptLoc), cl::ValueRequired,
            cl::ZeroOrMore);

bool DiagnosticHandler::isPassedRemarkEnabled(StringRef PassName) {
    return PassRemarksOptLoc.Pattern &&
         PassRemarksOptLoc.Pattern->match(PassName);
}

tools/llc/llc.cpp:

Context.setDiagnosticHandler(make_unique<DiagnosticHandler>());

tools/clang/lib/CodeGen/CodeGenAction.cpp:

class ClangDiagnosticHandler final : public DiagnosticHandler {
public:
  ClangDiagnosticHandler(const CodeGenOptions &CodeGenOpts) ...

  bool isAnalysisRemarkEnabled(StringRef PassName) override {
        return (CodeGenOpts.OptimizationRemarkPattern &&
                    CodeGenOpts.OptimizationRemarkPattern->match(PassName));
  }
  ...

private:
  const CodeGenOptions &CodeGenOpts;
};

...

  Ctx.setDiagnosticHandler(make_unique<ClangDiagnosticHandler>(CodeGenOpts));

Update the patch.

This is getting close so besides the comments below, please also start cleaning up the patch so that the diff is as tight as possible. Thanks!

include/llvm/Analysis/OptimizationDiagnosticInfo.h
80–81

Take a StringRef.

85–87

No {} after else.

include/llvm/CodeGen/MachineOptimizationRemarkEmitter.h
78

No need for .str()

include/llvm/IR/DiagnosticHandler.h
30

struct?

34–39

There should be a comment here explaining the relationship of DiagHandlerCB and handleDiagnostics. I.e. DiagHandler is settable from the C API and default definition of handleDiagnostics calls this. On the other hand anything subclassing DiagHandler is not required to call the CB.

35

As per my allowExtraAnalysis comment, I would drop this for now along with the RemarkInfo struct. We can add this later if we need it.

39

I don't think this should take a DiagContext, since none of the overriding subclasses use it.

To satisfy the C API, DiagContext should be move to this class from the LLVMContext as well.

So the original version of handleDiagnostics should just call DiagHandlerCB(DI, DiagContextCB).

include/llvm/IR/DiagnosticInfo.h
18–28

Don't reorder these (i.e. use clang-format on the diff) and don't include anything you don't really need.

122

This is the same thing just written differently.

include/llvm/IR/LLVMContext.h
200–212

See my comments on the C API, I am hoping we can remove this,

include/llvm/LTO/Config.h
174–180

This should take the DiagnosticHandlerFunction in the ctor, stash it and then call it in handleDiagnostics().

183–187

Can you please add a FIXME here. I think that if Config were to use a unique_ptr of DiagnosticHandler class the ownership would be properly passed. This is not for this patch, but would be good to note down.

lib/Transforms/Vectorize/LoopVectorize.cpp
4969–4971

This case suggest that we should just make allowExtraAnalysis return a bool if any kind of remark is enabled for this pass. Then this can just be invoked as:

if (ORE->allowExtraAnalysis(DEBUG_TYPE))
   ...
vivekvpandya marked 12 inline comments as done.

Updated the patch!

anemet added a subscriber: ab.Sep 1 2017, 11:23 PM
anemet added inline comments.
include/llvm/Analysis/OptimizationDiagnosticInfo.h
80–81

Just StringRef, no need for const &, it's already a const ref. You have bunch of this in the patch.

81–83
83

You could also just return the boolean expression directly, no need for the if.

include/llvm/IR/DiagnosticHandler.h
16–34

Each function or public member needs a comment.

In particular, explain the return value for handleDiagnostics. I.e. true means the Diagnostics was handled (otherwise the default handler is executed).

18

New line before this.

21–22

DiagnosticHandler(void *DiagContext == nullptr)

and then you don't need the other ctor.

30

isAnyRemarkEnabled

Please fix all these names to use Enabled rather than Enable

You may also want move the definition here so that it gets inlined.

include/llvm/IR/DiagnosticInfo.h
36

This does not seem related to this change. Remove or commit separately.

include/llvm/IR/LLVMContext.h
18

clang-format to get this header in the right position (headers are alphabetically sorted).

200–211

Fix this weird / on each line.

222

Needs to return a const pointer if it's a const member function.

include/llvm/LTO/Config.h
174–180

You didn't fix this. To be clear this should take a DiagnosticHandlerFunction in the ctor and have a field t o store it rather than abusing Context.

lib/IR/DiagnosticInfo.cpp
235

I don't think you need the .str(). You have a bunch of this.

lib/IR/LLVMContext.cpp
223–226

Don't change these unrelated lines.

224–228

Make this a single if?

lib/LTO/LTOCodeGenerator.cpp
656–664

Here too, no need to abuse the Context for the pointer. Just have a new field for it in this subclass.

tools/llc/llc.cpp
163

Again don't abuse Context but have a field for HasError in this class.

237–238

Don't rename, remove. Also move the new class down here so that the diff is easier to read.

tools/llvm-dis/llvm-dis.cpp
129

Again have a field for this rather than using Context.

tools/llvm-link/llvm-link.cpp
184–185

Remove

tools/llvm-lto/llvm-lto.cpp
237–238

Remove.

866

Remove unrelated change.

You also need to add a test. You should be able to extend either the LICM's or the Vectorizer's test to also get the remarks due to allowExtraAnalysis with -pass-remarks not just with -pass-remarks-output.

vivekvpandya marked 21 inline comments as done.

Updated the patch , tests will be updated in next patch.

You also need to add a test. You should be able to extend either the LICM's or the Vectorizer's test to also get the remarks due to allowExtraAnalysis with -pass-remarks not just with -pass-remarks-output.

If I understand this correctly then we need to remove -pass-remarks-output from all test which uses it just to return allowExtraAnalysis() true right? Also I think these kind of test uses llc .

include/llvm/IR/LLVMContext.h
18

clang-format -sort-includes gives me same result.

200–212

I don't think so as IR/Core.cpp uses this method to provide implantation of LLVM C API.

You also need to add a test. You should be able to extend either the LICM's or the Vectorizer's test to also get the remarks due to allowExtraAnalysis with -pass-remarks not just with -pass-remarks-output.

If I understand this correctly then we need to remove -pass-remarks-output from all test which uses it just to return allowExtraAnalysis() true right? Also I think these kind of test uses llc .

Not really. The test is actually GVN: test/Transforms/GVN/opt-remarks.ll. I just made some improvements in to make it more suitable to provide coverage for you these changes. Now the test should fail after your patch and you'd have to update the opt output testing. It should now print the "not eliminated" lines as well.

include/llvm/IR/LLVMContext.h
18

Perhaps written that way but it needs to be written as llvm/IR/DiagnosticHandler.h

200–212

Sorry that was an outdated comment that I've never finished, please ignore.

Not really. The test is actually GVN: test/Transforms/GVN/opt-remarks.ll. I just made some improvements in

... in r312544.

Test Updated

@anemet if this looks oky to you then I can commit this on weekend.

vivekvpandya edited the summary of this revision. (Show Details)Sep 8 2017, 10:05 AM

Only minor things at this point. This is very close now.

include/llvm/Analysis/OptimizationDiagnosticInfo.h
81–83

Why rvalue reference, you should just take this by value. There is no ownership transfer here.

84

No std::move here. You have more of this later.

include/llvm/IR/DiagnosticHandler.h
14

I don't think you need this.

19

Please add a comment before the class.

20

No need for public for struct.

28

implementation

46

Capitalize first word and end with period; comments are full sentences.

58

I would drop the flag names here since if those are overridden this is not true. Just say "Return true if any remark type is enabled."

lib/IR/DiagnosticInfo.cpp
44–45

Is this still used here?

lib/LTO/LTOCodeGenerator.cpp
667–668

I think that this comment still applies.

tools/llvm-lto/llvm-lto.cpp
67–100

Don't move this code unless you have to. The diff is easier to read that way.

vivekvpandya marked 8 inline comments as done.
anemet accepted this revision.Sep 11 2017, 9:08 PM

LGTM with the nits below. Thanks!

include/llvm/IR/DiagnosticHandler.h
2

DiagnosticHandler.h

lib/IR/LLVMContext.cpp
197

Remove this whitespace change

lib/IR/LLVMContextImpl.cpp
25–26

No need to pass nullptr here.

This revision is now accepted and ready to land.Sep 11 2017, 9:08 PM
vivekvpandya marked 3 inline comments as done.

Update.

Added method to detach unique_ptr<DiagnosticHandler> from LLVMContext.

anemet accepted this revision.Sep 13 2017, 4:12 PM

Still looks good.

lib/IR/LLVMContext.cpp
332

Has this version of the diff been clang-formatted?

Fixed in rL313456.

include/llvm/IR/DiagnosticHandler.h
12

Did you forget include guard here?