-
Notifications
You must be signed in to change notification settings - Fork 788
[DAE][SYCL] Enable DAE in SYCL kernel functions #2226
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 1 commit
39e166d
18e7bd0
5b0c754
e30b2bf
ab6f272
bcbbbb1
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 |
---|---|---|
|
@@ -41,6 +41,7 @@ | |
#include "llvm/InitializePasses.h" | ||
#include "llvm/Pass.h" | ||
#include "llvm/Support/Casting.h" | ||
#include "llvm/Support/CommandLine.h" | ||
#include "llvm/Support/Debug.h" | ||
#include "llvm/Support/raw_ostream.h" | ||
#include "llvm/Transforms/IPO.h" | ||
|
@@ -54,6 +55,11 @@ using namespace llvm; | |
|
||
#define DEBUG_TYPE "deadargelim" | ||
|
||
static cl::opt<std::string> | ||
IntegrationHeaderFileName("integr-header-file", | ||
cl::desc("Path to integration header file"), | ||
cl::value_desc("filename"), cl::Hidden); | ||
|
||
STATISTIC(NumArgumentsEliminated, "Number of unread args removed"); | ||
STATISTIC(NumRetValsEliminated , "Number of unused return values removed"); | ||
STATISTIC(NumArgumentsReplacedWithUndef, | ||
|
@@ -77,13 +83,15 @@ namespace { | |
bool runOnModule(Module &M) override { | ||
if (skipModule(M)) | ||
return false; | ||
DeadArgumentEliminationPass DAEP(ShouldHackArguments()); | ||
DeadArgumentEliminationPass DAEP(ShouldHackArguments(), | ||
CheckSyclKernels()); | ||
ModuleAnalysisManager DummyMAM; | ||
PreservedAnalyses PA = DAEP.run(M, DummyMAM); | ||
return !PA.areAllPreserved(); | ||
} | ||
|
||
virtual bool ShouldHackArguments() const { return false; } | ||
virtual bool CheckSyclKernels() const { return false; } | ||
}; | ||
|
||
} // end anonymous namespace | ||
|
@@ -103,6 +111,7 @@ namespace { | |
DAH() : DAE(ID) {} | ||
|
||
bool ShouldHackArguments() const override { return true; } | ||
bool CheckSyclKernels() const override { return false; } | ||
}; | ||
|
||
} // end anonymous namespace | ||
|
@@ -113,12 +122,40 @@ INITIALIZE_PASS(DAH, "deadarghaX0r", | |
"Dead Argument Hacking (BUGPOINT USE ONLY; DO NOT USE)", | ||
false, false) | ||
|
||
namespace { | ||
|
||
/// DAESYCL - DeadArgumentElimination pass for SYCL kernel functions even | ||
/// if they are external. | ||
struct DAESYCL : public DAE { | ||
static char ID; | ||
|
||
DAESYCL() : DAE(ID) { | ||
initializeDAESYCLPass(*PassRegistry::getPassRegistry()); | ||
} | ||
|
||
StringRef getPassName() const override { | ||
return "Dead Argument Elimination for SYCL kernels"; | ||
} | ||
|
||
bool ShouldHackArguments() const override { return false; } | ||
bool CheckSyclKernels() const override { return true; } | ||
}; | ||
|
||
} // end anonymous namespace | ||
|
||
char DAESYCL::ID = 0; | ||
|
||
INITIALIZE_PASS(DAESYCL, "deadargelim-sycl", | ||
"Dead Argument Elimination for SYCL kernels", false, false) | ||
|
||
/// createDeadArgEliminationPass - This pass removes arguments from functions | ||
/// which are not used by the body of the function. | ||
ModulePass *llvm::createDeadArgEliminationPass() { return new DAE(); } | ||
|
||
ModulePass *llvm::createDeadArgHackingPass() { return new DAH(); } | ||
|
||
ModulePass *llvm::createDeadArgEliminationSYCLPass() { return new DAESYCL(); } | ||
|
||
/// DeleteDeadVarargs - If this is an function that takes a ... list, and if | ||
/// llvm.vastart is never called, the varargs list is dead for the function. | ||
bool DeadArgumentEliminationPass::DeleteDeadVarargs(Function &Fn) { | ||
|
@@ -535,7 +572,14 @@ void DeadArgumentEliminationPass::SurveyFunction(const Function &F) { | |
<< " has musttail calls\n"); | ||
} | ||
|
||
if (!F.hasLocalLinkage() && (!ShouldHackArguments || F.isIntrinsic())) { | ||
// We can't modify arguments if the function is not local | ||
// but we can do so for SYCL kernel function. | ||
DenisBakhvalov marked this conversation as resolved.
Show resolved
Hide resolved
|
||
bool FuncIsSyclKernel = | ||
CheckSyclKernels && | ||
StringRef(F.getParent()->getTargetTriple()).contains("sycldevice") && | ||
F.getCallingConv() == CallingConv::SPIR_KERNEL; | ||
bool FuncIsLive = !F.hasLocalLinkage() && !FuncIsSyclKernel; | ||
if (FuncIsLive && (!ShouldHackArguments || F.isIntrinsic())) { | ||
MarkLive(F); | ||
return; | ||
} | ||
|
@@ -714,6 +758,83 @@ void DeadArgumentEliminationPass::PropagateLiveness(const RetOrArg &RA) { | |
Uses.erase(Begin, I); | ||
} | ||
|
||
// Update kernel arguments table inside the integration header. | ||
// For example: | ||
// static constexpr const bool param_omit_table[] = { | ||
// // OMIT_TABLE_BEGIN | ||
// // kernel_name_1 | ||
// false, false, // <= update to true if the argument is dead | ||
// // kernel_name_2 | ||
// false, false, | ||
// // OMIT_TABLE_END | ||
// }; | ||
// TODO: batch changes to multiple SYCL kernels and do one bulk update. | ||
constexpr StringLiteral OMIT_TABLE_BEGIN("// OMIT_TABLE_BEGIN"); | ||
constexpr StringLiteral OMIT_TABLE_END("// OMIT_TABLE_END"); | ||
static void updateIntegrationHeader(StringRef SyclKernelName, | ||
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. Looks OK...hopefully the header file is short and/or the number of kernels is small, as we are writing the whole file on each update. Otherwise we have to go with a "F F F F" kind of table that can be modified in-place without changing the size. 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. I don't think it will make a difference in practice. But I'm working on a patch for doing a bulk update of int-header. I would like to do this change incrementally, i.e. in a separate patch. |
||
const ArrayRef<bool> &ArgAlive) { | ||
// TODO: Enable asserts once the driver will provide the option. | ||
bader marked this conversation as resolved.
Show resolved
Hide resolved
|
||
if (!IntegrationHeaderFileName.getNumOccurrences()) | ||
return; | ||
// assert(IntegrationHeaderFileName.hasArgStr() && | ||
// "Integration header file not specified!"); | ||
ErrorOr<std::unique_ptr<MemoryBuffer>> IntHeaderBuffer = | ||
MemoryBuffer::getFile(IntegrationHeaderFileName); | ||
|
||
if (!IntHeaderBuffer) | ||
report_fatal_error("unable to read integration header file '" + | ||
IntegrationHeaderFileName + | ||
"': " + IntHeaderBuffer.getError().message()); | ||
Comment on lines
+781
to
+784
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. Are you going to replace this with assert(s)? 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. I think it is a hard fail. If we did not update int header it will result in a later runtime fail. 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.
If we keep all arguments, it should be okay. I mean if anything is wrong with pre-requisites, this pass can just do nothing instead of crashing and it won't break anything. 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. Introduced an early exit if 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. Great. I think we can replace all error checking with asserts in this file. if (!<cond>)
report_fatal_error(<msg>); -> assert(<cond> & <msg>); I don't think we should do thorough runtime checking for integration header format. This file is auto-generated by the compiler, so it's should be validated in scope of #2236. We can validate that file format is correct with asserts. 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. Feel free to address this in a separate PR. |
||
|
||
// 1. Find the region between OMIT_TABLE_BEGIN and OMIT_TABLE_END | ||
StringRef IntHeader((*IntHeaderBuffer)->getBuffer()); | ||
if (!IntHeader.contains(OMIT_TABLE_BEGIN)) | ||
report_fatal_error(OMIT_TABLE_BEGIN + | ||
" marker not found in integration header"); | ||
if (!IntHeader.contains(OMIT_TABLE_END)) | ||
report_fatal_error(OMIT_TABLE_END + | ||
" marker not found in integration header"); | ||
|
||
size_t BeginRegionPos = | ||
IntHeader.find(OMIT_TABLE_BEGIN) + OMIT_TABLE_BEGIN.size(); | ||
size_t EndRegionPos = IntHeader.find(OMIT_TABLE_END); | ||
|
||
StringRef OmitArgTable = IntHeader.slice(BeginRegionPos, EndRegionPos); | ||
|
||
// 2. Find the line that corresponds to the SYCL kernel | ||
if (!OmitArgTable.contains(SyclKernelName)) | ||
report_fatal_error( | ||
"Argument table not found in integration header for function '" + | ||
SyclKernelName + "'"); | ||
|
||
size_t BeginLinePos = | ||
OmitArgTable.find(SyclKernelName) + SyclKernelName.size(); | ||
size_t EndLinePos = OmitArgTable.find("//", BeginLinePos); | ||
|
||
StringRef OmitArgLine = OmitArgTable.slice(BeginLinePos, EndLinePos); | ||
|
||
size_t LineLeftTrim = OmitArgLine.size() - OmitArgLine.ltrim().size(); | ||
size_t LineRightTrim = OmitArgLine.size() - OmitArgLine.rtrim().size(); | ||
|
||
// 3. Construct new file contents and replace only that string. | ||
std::string NewIntHeader; | ||
NewIntHeader += | ||
IntHeader.take_front(BeginRegionPos + BeginLinePos + LineLeftTrim); | ||
for (auto &AliveArg : ArgAlive) | ||
NewIntHeader += AliveArg ? "false, " : "true, "; | ||
NewIntHeader += IntHeader.drop_front(BeginRegionPos + BeginLinePos + | ||
OmitArgLine.size() - LineRightTrim); | ||
|
||
// 4. Flush the string into the file. | ||
std::error_code EC; | ||
raw_fd_ostream File(IntegrationHeaderFileName, EC, sys::fs::F_Text); | ||
|
||
if (EC) | ||
report_fatal_error("Cannot open integration header for writing."); | ||
|
||
File << NewIntHeader; | ||
} | ||
|
||
// RemoveDeadStuffFromFunction - Remove any arguments and return values from F | ||
// that are not in LiveValues. Transform the function and all of the callees of | ||
// the function to not have these arguments and return values. | ||
|
@@ -757,6 +878,9 @@ bool DeadArgumentEliminationPass::RemoveDeadStuffFromFunction(Function *F) { | |
} | ||
} | ||
|
||
if (CheckSyclKernels) | ||
updateIntegrationHeader(F->getName(), ArgAlive); | ||
|
||
// Find out the new return value. | ||
Type *RetTy = FTy->getReturnType(); | ||
Type *NRetTy = nullptr; | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,13 @@ | ||
; TODO: this should be crashing (not --crash) after enabling assert | ||
bader marked this conversation as resolved.
Show resolved
Hide resolved
|
||
; RUN: opt < %s -deadargelim-sycl -S | ||
|
||
; Path to the integration header is not specified. | ||
|
||
target triple = "spir64-unknown-unknown-sycldevice" | ||
|
||
define weak_odr spir_kernel void @NegativeSyclKernel(float %arg1, float %arg2) { | ||
call void @foo(float %arg1) | ||
ret void | ||
} | ||
|
||
declare void @foo(float %arg) |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,14 @@ | ||
; RUN: rm -rf %t && mkdir -p %t | ||
DenisBakhvalov marked this conversation as resolved.
Show resolved
Hide resolved
|
||
; RUN: touch %t/int_header.h | ||
; RUN: not --crash opt < %s -deadargelim-sycl -S -integr-header-file %t/bad_file.h | ||
|
||
; Path to the integration header is wrong. | ||
|
||
target triple = "spir64-unknown-unknown-sycldevice" | ||
|
||
define weak_odr spir_kernel void @NegativeSyclKernel(float %arg1, float %arg2) { | ||
call void @foo(float %arg1) | ||
ret void | ||
} | ||
|
||
declare void @foo(float %arg) |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,17 @@ | ||
; RUN: rm -rf %t && mkdir -p %t | ||
; RUN: echo 'static constexpr const bool param_omit_table[] = {' >> %t/int_header.h | ||
; RUN: echo ' // NegativeSyclKernel' >> %t/int_header.h | ||
; RUN: echo ' false, false,' >> %t/int_header.h | ||
; RUN: echo '};' >> %t/int_header.h | ||
; RUN: not --crash opt < %s -deadargelim-sycl -S -integr-header-file %t/int_header.h | ||
|
||
; No OMIT_TABLE markers in the integration header. | ||
|
||
target triple = "spir64-unknown-unknown-sycldevice" | ||
|
||
define weak_odr spir_kernel void @NegativeSyclKernel(float %arg1, float %arg2) { | ||
call void @foo(float %arg1) | ||
ret void | ||
} | ||
|
||
declare void @foo(float %arg) |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,19 @@ | ||
; RUN: rm -rf %t && mkdir -p %t | ||
; RUN: echo 'static constexpr const bool param_omit_table[] = {' >> %t/int_header.h | ||
; RUN: echo ' // OMIT_TABLE_BEGIN' >> %t/int_header.h | ||
; RUN: echo ' // WrongKernelName' >> %t/int_header.h | ||
; RUN: echo ' false, false,' >> %t/int_header.h | ||
; RUN: echo ' // OMIT_TABLE_END' >> %t/int_header.h | ||
; RUN: echo '};' >> %t/int_header.h | ||
; RUN: not --crash opt < %s -deadargelim-sycl -S -integr-header-file %t/int_header.h | ||
|
||
; Wrong kernel name in the integration header. | ||
|
||
target triple = "spir64-unknown-unknown-sycldevice" | ||
|
||
define weak_odr spir_kernel void @NegativeSyclKernel(float %arg1, float %arg2) { | ||
call void @foo(float %arg1) | ||
ret void | ||
} | ||
|
||
declare void @foo(float %arg) |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,20 @@ | ||
; NOTE: Assertions have been autogenerated by utils/update_test_checks.py UTC_ARGS: --function-signature | ||
; RUN: opt < %s -deadargelim -S | FileCheck %s | ||
; RUN: opt < %s -deadargelim-sycl -S | FileCheck %s | ||
|
||
; This test ensures dead arguments are not eliminated | ||
; from a global function that is not a SYCL kernel. | ||
|
||
target triple = "spir64-unknown-unknown-sycldevice" | ||
|
||
define weak_odr void @NotASyclKernel(float %arg1, float %arg2) { | ||
; CHECK-LABEL: define {{[^@]+}}@NotASyclKernel | ||
; CHECK-SAME: (float [[ARG1:%.*]], float [[ARG2:%.*]]) | ||
; CHECK-NEXT: call void @foo(float [[ARG1]]) | ||
; CHECK-NEXT: ret void | ||
; | ||
call void @foo(float %arg1) | ||
ret void | ||
} | ||
|
||
declare void @foo(float %arg) |
Uh oh!
There was an error while loading. Please reload this page.