-
Notifications
You must be signed in to change notification settings - Fork 769
Generate an opt report of kernel arguments. #3492
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from 18 commits
d99d3a7
4845eb1
8b5d07c
e69616e
5d12df4
c44d670
87ce263
232a8a9
4be5deb
978d6b8
1db1025
82cacc9
5254539
32454ac
d316062
95b6c9f
aeed807
1287604
8737bc1
c5d2ae3
6ce7268
f33353d
ddad6f9
0af4422
2c0a35c
f99e200
0937042
6a87209
eac6f25
c04374c
84a2cee
8efb79a
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change | ||||
---|---|---|---|---|---|---|
@@ -0,0 +1,57 @@ | ||||||
//===---------------------- SyclOptReport.h ---------------------*- C++ -*-===// | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Filename is SyclOptReportHandler.h There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Can you please change this as well? |
||||||
// | ||||||
// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions. | ||||||
// See https://llvm.org/LICENSE.txt for license information. | ||||||
// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception | ||||||
// | ||||||
//===----------------------------------------------------------------------===// | ||||||
/// | ||||||
/// \file | ||||||
/// Defines clang::SyclOptReport class. | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. SyclOptReportHandler* |
||||||
/// | ||||||
//===----------------------------------------------------------------------===// | ||||||
|
||||||
#ifndef LLVM_CLANG_BASIC_SYCLOPTREPORTHANDLER_H | ||||||
#define LLVM_CLANG_BASIC_SYCLOPTREPORTHANDLER_H | ||||||
|
||||||
#include "clang/Basic/SourceLocation.h" | ||||||
#include "llvm/ADT/DenseMap.h" | ||||||
#include "llvm/ADT/StringRef.h" | ||||||
|
||||||
namespace clang { | ||||||
|
||||||
class FunctionDecl; | ||||||
|
||||||
class SyclOptReportHandler { | ||||||
private: | ||||||
struct OptReportInfo { | ||||||
StringRef KernelArgName; | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This is a use-after-free waiting to happen -- There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Should KernelArgName be a std::unique_ptr? |
||||||
std::string KernelArgType; | ||||||
SourceLocation KernelArgLoc; | ||||||
|
||||||
OptReportInfo(llvm::StringRef ArgName, std::string ArgType, | ||||||
SourceLocation ArgLoc) | ||||||
: KernelArgName(ArgName), KernelArgType(ArgType), KernelArgLoc(ArgLoc) { | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Suggested change
Might as well use the move constructors. |
||||||
} | ||||||
}; | ||||||
llvm::DenseMap<const FunctionDecl *, SmallVector<OptReportInfo, 4>> Map; | ||||||
|
||||||
public: | ||||||
void AddKernelArgs(const FunctionDecl *FD, StringRef ArgName, | ||||||
std::string ArgType, SourceLocation ArgLoc) { | ||||||
Map[FD].emplace_back(ArgName, ArgType, ArgLoc); | ||||||
} | ||||||
SmallVector<OptReportInfo, 4> &getInfo(const FunctionDecl *FD) { | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Suggested change
|
||||||
auto It = Map.find(FD); | ||||||
assert(It != Map.end()); | ||||||
return It->second; | ||||||
} | ||||||
|
||||||
bool HasOptReportInfo(const FunctionDecl *FD) { | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Suggested change
|
||||||
return Map.find(FD) != Map.end(); | ||||||
} | ||||||
}; | ||||||
|
||||||
} // namespace clang | ||||||
|
||||||
#endif // LLVM_CLANG_BASIC_SYCLOPTREPORTHANDLER_H |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -14,7 +14,10 @@ | |
#include "CodeGenFunction.h" | ||
#include "clang/AST/Attr.h" | ||
#include "clang/AST/Decl.h" | ||
#include "clang/Basic/SourceLocation.h" | ||
#include "llvm/Analysis/OptimizationRemarkEmitter.h" | ||
#include "llvm/IR/Instructions.h" | ||
|
||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Spurious newline? |
||
#include <assert.h> | ||
|
||
using namespace clang; | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -304,28 +304,12 @@ namespace clang { | |
return; | ||
|
||
LLVMContext &Ctx = getModule()->getContext(); | ||
|
||
std::unique_ptr<DiagnosticHandler> OldDiagnosticHandler = | ||
Ctx.getDiagnosticHandler(); | ||
Ctx.setDiagnosticHandler(std::make_unique<ClangDiagnosticHandler>( | ||
CodeGenOpts, this)); | ||
|
||
Expected<std::unique_ptr<llvm::ToolOutputFile>> OptRecordFileOrErr = | ||
setupLLVMOptimizationRemarks( | ||
Ctx, CodeGenOpts.OptRecordFile, CodeGenOpts.OptRecordPasses, | ||
CodeGenOpts.OptRecordFormat, CodeGenOpts.DiagnosticsWithHotness, | ||
CodeGenOpts.DiagnosticsHotnessThreshold); | ||
|
||
if (Error E = OptRecordFileOrErr.takeError()) { | ||
reportOptRecordError(std::move(E), Diags, CodeGenOpts); | ||
return; | ||
} | ||
|
||
std::unique_ptr<llvm::ToolOutputFile> OptRecordFile = | ||
std::move(*OptRecordFileOrErr); | ||
|
||
if (OptRecordFile && | ||
CodeGenOpts.getProfileUse() != CodeGenOptions::ProfileNone) | ||
Ctx.setDiagnosticsHotnessRequested(true); | ||
// The diagnostic handler is now processed in OptRecordFileRAII. | ||
|
||
// The parallel_for_work_group legalization pass can emit calls to | ||
// builtins function. Definitions of those builtins can be provided in | ||
|
@@ -349,9 +333,6 @@ namespace clang { | |
getModule(), Action, std::move(AsmOutStream)); | ||
|
||
Ctx.setDiagnosticHandler(std::move(OldDiagnosticHandler)); | ||
|
||
if (OptRecordFile) | ||
OptRecordFile->keep(); | ||
} | ||
|
||
void HandleTagDeclDefinition(TagDecl *D) override { | ||
|
@@ -1046,8 +1027,52 @@ CodeGenAction::loadModule(MemoryBufferRef MBRef) { | |
return {}; | ||
} | ||
|
||
namespace { | ||
// Handles the initialization and cleanup of the OptRecordFile. This | ||
// customization allows initialization before the clang codegen runs | ||
// so it can also emit to the opt report. | ||
struct OptRecordFileRAII { | ||
std::unique_ptr<llvm::ToolOutputFile> OptRecordFile; | ||
std::unique_ptr<DiagnosticHandler> OldDiagnosticHandler; | ||
llvm::LLVMContext &Ctx; | ||
|
||
OptRecordFileRAII(CodeGenAction &CGA, llvm::LLVMContext &Ctx, | ||
BackendConsumer &BC) | ||
: Ctx(Ctx) { | ||
CompilerInstance &CI = CGA.getCompilerInstance(); | ||
auto &CodeGenOpts = CI.getCodeGenOpts(); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Please spell out the type. |
||
|
||
OldDiagnosticHandler = Ctx.getDiagnosticHandler(); | ||
|
||
Ctx.setDiagnosticHandler( | ||
std::make_unique<ClangDiagnosticHandler>(CodeGenOpts, &BC)); | ||
|
||
Expected<std::unique_ptr<llvm::ToolOutputFile>> OptRecordFileOrErr = | ||
setupLLVMOptimizationRemarks( | ||
Ctx, CodeGenOpts.OptRecordFile, CodeGenOpts.OptRecordPasses, | ||
CodeGenOpts.OptRecordFormat, CodeGenOpts.DiagnosticsWithHotness, | ||
CodeGenOpts.DiagnosticsHotnessThreshold); | ||
|
||
if (Error E = OptRecordFileOrErr.takeError()) | ||
reportOptRecordError(std::move(E), CI.getDiagnostics(), CodeGenOpts); | ||
else | ||
OptRecordFile = std::move(*OptRecordFileOrErr); | ||
|
||
if (OptRecordFile && | ||
CodeGenOpts.getProfileUse() != CodeGenOptions::ProfileNone) | ||
Ctx.setDiagnosticsHotnessRequested(true); | ||
} | ||
~OptRecordFileRAII() { | ||
Ctx.setDiagnosticHandler(std::move(OldDiagnosticHandler)); | ||
if (OptRecordFile) | ||
OptRecordFile->keep(); | ||
} | ||
}; | ||
} // namespace | ||
|
||
void CodeGenAction::ExecuteAction() { | ||
if (getCurrentFileKind().getLanguage() != Language::LLVM_IR) { | ||
OptRecordFileRAII ORF(*this, *VMContext, *BEConsumer); | ||
this->ASTFrontendAction::ExecuteAction(); | ||
return; | ||
} | ||
|
Original file line number | Diff line number | Diff line change | ||||
---|---|---|---|---|---|---|
|
@@ -35,6 +35,7 @@ | |||||
#include "clang/CodeGen/CGFunctionInfo.h" | ||||||
#include "clang/Frontend/FrontendDiagnostic.h" | ||||||
#include "llvm/ADT/ArrayRef.h" | ||||||
#include "llvm/Analysis/OptimizationRemarkEmitter.h" | ||||||
#include "llvm/Frontend/OpenMP/OMPIRBuilder.h" | ||||||
#include "llvm/IR/DataLayout.h" | ||||||
#include "llvm/IR/Dominators.h" | ||||||
|
@@ -1521,6 +1522,25 @@ void CodeGenFunction::GenerateCode(GlobalDecl GD, llvm::Function *Fn, | |||||
// Emit the standard function prologue. | ||||||
StartFunction(GD, ResTy, Fn, FnInfo, Args, Loc, BodyRange.getBegin()); | ||||||
|
||||||
SyclOptReportHandler &OptReportHandler = CGM.getDiags().OptReportHandler; | ||||||
if (OptReportHandler.HasOptReportInfo(FD)) { | ||||||
AaronBallman marked this conversation as resolved.
Show resolved
Hide resolved
|
||||||
llvm::OptimizationRemarkEmitter ORE(Fn); | ||||||
int count = 0; | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Suggested change
|
||||||
for (auto &ORI : OptReportHandler.getInfo(FD)) { | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Suggested change
|
||||||
llvm::DiagnosticLocation DL = SourceLocToDebugLoc(ORI.KernelArgLoc); | ||||||
llvm::OptimizationRemark Remark("sycl", "Region", DL, | ||||||
&Fn->getEntryBlock()); | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. We seem to calling ORI.value() multiple times below; perhaps save ORI.value().KernelArgName in a variable here and the reuse that? |
||||||
Remark << "Argument " << llvm::ore::NV("Argument", count++) | ||||||
<< " for function kernel: " | ||||||
<< llvm::ore::NV(ORI.KernelArgName.empty() ? "&" : "") << " " | ||||||
<< Fn->getName() << "." | ||||||
<< llvm::ore::NV(ORI.KernelArgName.empty() ? " " | ||||||
: ORI.KernelArgName) | ||||||
<< "(" << ORI.KernelArgType << ")"; | ||||||
ORE.emit(Remark); | ||||||
} | ||||||
} | ||||||
|
||||||
// Generate the body of the function. | ||||||
PGO.assignRegionCounters(GD, CurFn); | ||||||
if (isa<CXXDestructorDecl>(FD)) | ||||||
|
@@ -2618,8 +2638,7 @@ Address CodeGenFunction::EmitIntelFPGAFieldAnnotations(SourceLocation Location, | |||||
// llvm.ptr.annotation intrinsic accepts a pointer to integer of any width - | ||||||
// don't perform bitcasts if value is integer | ||||||
if (VTy->getPointerElementType()->isIntegerTy()) { | ||||||
llvm::Function *F = | ||||||
CGM.getIntrinsic(llvm::Intrinsic::ptr_annotation, VTy); | ||||||
llvm::Function *F = CGM.getIntrinsic(llvm::Intrinsic::ptr_annotation, VTy); | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Unrelated change? |
||||||
V = EmitAnnotationCall(F, V, AnnotStr, Location); | ||||||
|
||||||
return Address(V, Addr.getAlignment()); | ||||||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,101 @@ | ||
// RUN: %clang_cc1 -triple spir64-unknown-unknown-sycldevice -fsycl-is-device \ | ||
// RUN: -Wno-sycl-2017-compat -emit-llvm-bc %s -o %t-host.bc -opt-record-file %t-host.yaml | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Could you expand this test to check for basic things in the opt-report file? I would like to see that to understand the changes in CodeGenFunction better. |
||
// RUN: FileCheck -check-prefix=CHECK --input-file %t-host.yaml %s | ||
// The test generates remarks about the kernel argument, their location and type | ||
// in the resulting yaml file. | ||
|
||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Also add a comment here describing what the test is testing for. |
||
#include "Inputs/sycl.hpp" | ||
|
||
class second_base { | ||
public: | ||
int *e; | ||
}; | ||
|
||
class InnerFieldBase { | ||
public: | ||
int d; | ||
}; | ||
class InnerField : public InnerFieldBase { | ||
int c; | ||
}; | ||
|
||
struct base { | ||
public: | ||
int b; | ||
InnerField obj; | ||
}; | ||
|
||
//CHECK: --- !Passed | ||
//CHECK: Pass:{{.*}}sycl | ||
//CHECK: Name:{{.*}}Region | ||
//CHECK: DebugLoc:{{.*}} { File: '{{.*}}kernel-arg-opt-report.cpp', | ||
//CHECK: Line: 85, Column: 18 } | ||
//CHECK: Function: _ZTS7derived | ||
//CHECK: Args: | ||
//CHECK-NEXT: String: 'Argument ' | ||
//CHECK-NEXT: Argument: '0' | ||
//CHECK-NEXT: String: ' for function kernel: ' | ||
//CHECK-NEXT: String: '&' | ||
//CHECK-NEXT: String: ' ' | ||
//CHECK-NEXT: String: _ZTS7derived | ||
//CHECK-NEXT: String: . | ||
//CHECK-NEXT: String: ' ' | ||
//CHECK-NEXT: String: '(' | ||
//CHECK-NEXT: String: struct base | ||
//CHECK-NEXT: String: ')' | ||
|
||
//CHECK: --- !Passed | ||
//CHECK: Pass:{{.*}}sycl | ||
//CHECK: Name:{{.*}}Region | ||
//CHECK: DebugLoc:{{.*}} { File: '{{.*}}kernel-arg-opt-report.cpp', | ||
//CHECK: Line: 11, Column: 8 } | ||
//CHECK: Function: _ZTS7derived | ||
//CHECK: Args: | ||
//CHECK-NEXT: String: 'Argument ' | ||
//CHECK-NEXT: Argument: '1' | ||
//CHECK-NEXT: String: ' for function kernel: ' | ||
//CHECK-NEXT: String: '' | ||
//CHECK-NEXT: String: ' ' | ||
//CHECK-NEXT: String: _ZTS7derived | ||
//CHECK-NEXT: String: . | ||
//CHECK-NEXT: String: e | ||
//CHECK-NEXT: String: '(' | ||
//CHECK-NEXT: String: struct __wrapper_class | ||
//CHECK-NEXT: String: ')' | ||
|
||
//CHECK: --- !Passed | ||
//CHECK: Pass:{{.*}}sycl | ||
//CHECK: Name:{{.*}}Region | ||
//CHECK: DebugLoc:{{.*}} { File: '{{.*}}kernel-arg-opt-report.cpp', | ||
//CHECK: Line: 86, Column: 7 } | ||
//CHECK: Function: _ZTS7derived | ||
//CHECK: Args: | ||
//CHECK-NEXT: String: 'Argument ' | ||
//CHECK-NEXT: Argument: '2' | ||
//CHECK-NEXT: String: ' for function kernel: ' | ||
//CHECK-NEXT: String: '' | ||
//CHECK-NEXT: String: ' ' | ||
//CHECK-NEXT: String: _ZTS7derived | ||
//CHECK-NEXT: String: . | ||
//CHECK-NEXT: String: a | ||
//CHECK-NEXT: String: '(' | ||
//CHECK-NEXT: String: int | ||
//CHECK-NEXT: String: ')' | ||
|
||
struct derived : base, second_base { | ||
int a; | ||
|
||
void operator()() const { | ||
} | ||
}; | ||
|
||
int main() { | ||
sycl::queue q; | ||
|
||
q.submit([&](cl::sycl::handler &cgh) { | ||
derived f{}; | ||
cgh.single_task(f); | ||
}); | ||
|
||
return 0; | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is there a reason this must be public?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It is being used in SemaSYCL.cpp referenced SemaRef.getDiagnostics().OptReportHandler.
It wouldn't be able to access private member.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It could still be a private data member with a public member accessor, which is a bit cleaner.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Getting still the same error. Making it private to DiagnosticEngine and make the data member to it public didn't seem to change anything. Am I missing something?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Did you try doing something similar to what getDiags() does for Diags in that class?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Tried a few things and changing the data member of SyclOptReportHandler to public seems to lead me to the same issue. Will keep looking at it.
getDiags() is public?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we may be crossing wires somewhere -- the suggestion is to convert:
into:
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Grrr! I was putting the declaration on the innermost private inside DiagnosticsEngine class, that's why it didn't work.