Skip to content

[InferAddressSpaces] Add InferAddressSpaces pass to pipeline for SPIR #7418

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.

Sign up for GitHub

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

Merged
merged 2 commits into from
Nov 21, 2022
Merged

[InferAddressSpaces] Add InferAddressSpaces pass to pipeline for SPIR #7418

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
3a38885
[InferAddressSpaces] Add InferAddressSpaces pass to pipeline for SPIR
bader Nov 16, 2022
1345fee
Add a comment to the lit test.
bader Nov 16, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
19 changes: 19 additions & 0 deletions clang/include/clang/Basic/Targets/SPIR.h
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,19 @@
//===---- SPIR.h - Declare SPIR and SPIR-V target interfaces ----*- C++ -*-===//
//
// 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
//
//===----------------------------------------------------------------------===//

#pragma once

namespace clang {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not sure if this is the right place for this. Can this information be added to /clang/lib/Basic/Targets/SPIR.h

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

+1

IMO, a better approach would be to expose a flat address space via TargetInfo which pulls the actual value from the actual target.

But I think the rational approach is to add a getFlatAddressSpace override to llvm/lib/Target/SPIRV/SPIRVTargetTransformInfo.h, this would make the infer pass pull the flat address space value from the TTI. The issue is this will force DPC++ to build the SPIR-V backend, it will have no impact on a normal sycl compilation flow but could result in users in invoking the wrong path if they tweak things.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not sure if this is the right place for this. Can this information be added to /clang/lib/Basic/Targets/SPIR.h

This information is already there. Unfortunately, we SPIR.h header is located in lib directory, so it's not available to lib/CodeGen sources.

But I think the rational approach is to add a getFlatAddressSpace override to llvm/lib/Target/SPIRV/SPIRVTargetTransformInfo.h, this would make the infer pass pull the flat address space value from the TTI. The issue is this will force DPC++ to build the SPIR-V backend, it will have no impact on a normal sycl compilation flow but could result in users in invoking the wrong path if they tweak things.

Thats right. I would like to use this approach, but today we target spir triple, which has no TTI. Once SPIR-V backend is matured enough to support SYCL, we can remove the duplication and use llvm/lib/Target/SPIRV/SPIRVTargetTransformInfo.h.

This code was written original author of the patch. See this commit - 7c28d7d. I can revert this commit to avoid adding a new header and use named constant in CodeGen lib sources. My assumption is this header is added to include with assumption that this information will be needed by other clang libraries. Let me know which option you prefer.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This information is already there. Unfortunately, we SPIR.h header is located in lib directory, so it's not available to lib/CodeGen sources.

I think you can access TargetInfo via Context in CodeGen. If you add an interface (similar to getConstantAddressSpace()) which you ovverride for SPIR target wouldn't it work? If not, can you please explain why ?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think EmitAssemblyHelper has no access to ASTContext or TargetInfo.

Copy link
Contributor

@elizabethandrews elizabethandrews Nov 21, 2022 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Right, my point was is there something stopping us from passing CodeGenModule/TargetInfo. Actually I just saw that data layout string is accessible where EmitAssemblyHelper is called. Is the information you need accessible via DataLayout string?

I'm not saying we need to do this by the way. I'm just trying to understand why we are not going this route if it is possible to do so. I found it odd that we need to add a 'Targets' folder in Basic when no other Target seems to require this. But I haven't worked as much with this side of the code so my thoughts may be incorrect. I don't want to hold up the code if @premanandrao @Naghasan and @smanna12 find current solution acceptable.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think EmitAssemblyHelper has no access to ASTContext or TargetInfo.

It doesn't as far as I can see. I see that as a work around anyway, I think might just be better to have that in a file name that doesn't clash with the rest. You could simply have it in EmitAssemblyHelper.

Is the information you need accessible via DataLayout string?

I checked, didn't see any reference.

I found it odd that we need to add a 'Targets' folder in Basic when no other Target seems to require this.

Other targets have this value pulled from the backend via the TargetTransformInfo (which is what I suggested to use), for instance the definition for AMDGCN.

I don't want to hold up the code

Same, I just have a reservation on the file name but still fine as is.

but today we target spir triple, which has no TTI. Once SPIR-V backend is matured enough to support SYCL, we can remove the duplication and use

I'm not suggesting to use the SPIR-V backend as a mean to produce the SPIR-V IR. And the driver won't do that anyway. It is just a case to build the SPIR-V backend so we can make use of target info, features etc.

SPIR could be added as one of the SPIR-V target (from dpc++, this is just a triple really). I know upstream has reservations, but as there is no other target that uses this triple, I don't really see the harm as SPIR is supposed to be used in conjunction with -emit-llvm (again, I'm not suggesting to use the backend to produce anything, just pull data and control generic transformations). We can also emit a warning / error if someone tries to emit SPIR-V using the backend with the SPIR triple if we are worried ppl start to use it as a "normal path".

I have other use cases where this would be extremely useful to do this but I'll stop here as I'm diverging way too much, probably best for an offline discussion.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

SPIR could be added as one of the SPIR-V target (from dpc++, this is just a triple really). I know upstream has reservations, but as there is no other target that uses this triple, I don't really see the harm as SPIR is supposed to be used in conjunction with -emit-llvm (again, I'm not suggesting to use the backend to produce anything, just pull data and control generic transformations).

Do you have any pointers to implementations of this approach? I don't think I've seen anything like this in the upstream.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've created a separate issue - #7670 to continue discussion there as this PR is merged.

namespace targets {

// Used by both the SPIR and SPIR-V targets. Code of the generic address space
// for the target
constexpr unsigned SPIR_GENERIC_AS = 4u;

} // namespace targets
} // namespace clang
13 changes: 12 additions & 1 deletion clang/lib/CodeGen/BackendUtil.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,7 @@
#include "clang/Basic/Diagnostic.h"
#include "clang/Basic/LangOptions.h"
#include "clang/Basic/TargetOptions.h"
#include "clang/Basic/Targets/SPIR.h"
#include "clang/Frontend/FrontendDiagnostic.h"
#include "clang/Frontend/Utils.h"
#include "clang/Lex/HeaderSearchOptions.h"
Expand Down Expand Up @@ -86,6 +87,7 @@
#include "llvm/Transforms/Scalar.h"
#include "llvm/Transforms/Scalar/EarlyCSE.h"
#include "llvm/Transforms/Scalar/GVN.h"
#include "llvm/Transforms/Scalar/InferAddressSpaces.h"
#include "llvm/Transforms/Scalar/JumpThreading.h"
#include "llvm/Transforms/Scalar/LowerMatrixIntrinsics.h"
#include "llvm/Transforms/Scalar/NewGVN.h"
Expand Down Expand Up @@ -894,6 +896,15 @@ void EmitAssemblyHelper::RunOptimizationPipeline(
MPM.addPass(SYCLPropagateAspectsUsagePass());
});

// Add the InferAddressSpaces pass for all the SPIR[V] targets
if (TargetTriple.isSPIR() || TargetTriple.isSPIRV()) {
PB.registerOptimizerLastEPCallback(
[](ModulePassManager &MPM, OptimizationLevel Level) {
MPM.addPass(createModuleToFunctionPassAdaptor(
InferAddressSpacesPass(clang::targets::SPIR_GENERIC_AS)));
});
}

bool IsThinLTO = CodeGenOpts.PrepareForThinLTO;
bool IsLTO = CodeGenOpts.PrepareForLTO;

Expand Down Expand Up @@ -996,7 +1007,7 @@ void EmitAssemblyHelper::RunOptimizationPipeline(
// -fsycl-instrument-device-code option was passed. This option can be used
// only with spir triple.
if (LangOpts.SYCLIsDevice && CodeGenOpts.SPIRITTAnnotations) {
assert(llvm::Triple(TheModule->getTargetTriple()).isSPIR() &&
assert(TargetTriple.isSPIR() &&
"ITT annotations can only be added to a module with spir target");
MPM.addPass(SPIRITTAnnotationsPass());
}
Expand Down
21 changes: 21 additions & 0 deletions clang/test/CodeGenSYCL/infer-address-spaces.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,21 @@
// RUN: %clang_cc1 -O1 -fsycl-is-device -internal-isystem %S/Inputs -triple spir64 -emit-llvm %s -o - | FileCheck %s

// Test that address spaces are deduced correctly by compiler optimizations.

#include "sycl.hpp"

using namespace sycl;

void foo(const float *usm_in, float* usm_out) {
queue Q;
Q.submit([&](handler &cgh) {
cgh.single_task<class test>([=](){
*usm_out = *usm_in;
});
});
}

// No addrspacecast before loading and storing values
// CHECK-NOT: addrspacecast
// CHECK: %0 = load float, ptr addrspace(1)
// CHECK: store float %0, ptr addrspace(1)