Skip to content

[SYCL][FPGA] Changing the attribute max_private_copies to private_copes #994

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

Closed
wants to merge 1 commit into from
Closed

[SYCL][FPGA] Changing the attribute max_private_copies to private_copes #994

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
6 changes: 3 additions & 3 deletions clang/include/clang/Basic/Attr.td
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1721,12 +1721,12 @@ def IntelFPGANumBanks : Attr {
}];
}

def IntelFPGAMaxPrivateCopies : InheritableAttr {
let Spellings = [CXX11<"intelfpga","max_private_copies">];
def IntelFPGAPrivateCopies : InheritableAttr {
let Spellings = [CXX11<"intelfpga","private_copies">];
let Args = [ExprArgument<"Value">];
let LangOpts = [SYCLIsDevice, SYCLIsHost];
let Subjects = SubjectList<[IntelFPGALocalNonConstVar, Field], ErrorDiag>;
let Documentation = [IntelFPGAMaxPrivateCopiesAttrDocs];
let Documentation = [IntelFPGAPrivateCopiesAttrDocs];
let AdditionalMembers = [{
static unsigned getMinValue() {
return 0;
Expand Down
4 changes: 2 additions & 2 deletions clang/include/clang/Basic/AttrDocs.td
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1876,9 +1876,9 @@ with N banks.
}];
}

def IntelFPGAMaxPrivateCopiesAttrDocs : Documentation {
def IntelFPGAPrivateCopiesAttrDocs : Documentation {
let Category = DocCatVariable;
let Heading = "max_private_copies (IntelFPGA)";
let Heading = "private_copies (IntelFPGA)";
let Content = [{
This attribute may be attached to a variable or struct member declaration and
instructs the backend to replicate the memory generated for the variable or
Expand Down
4 changes: 2 additions & 2 deletions clang/lib/CodeGen/CodeGenModule.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3975,9 +3975,9 @@ void CodeGenModule::generateIntelFPGAAnnotation(
llvm::APSInt BWAInt = BWA->getValue()->EvaluateKnownConstInt(getContext());
Out << '{' << BWA->getSpelling() << ':' << BWAInt << '}';
}
if (const auto *MCA = D->getAttr<IntelFPGAMaxPrivateCopiesAttr>()) {
if (const auto *MCA = D->getAttr<IntelFPGAPrivateCopiesAttr>()) {
llvm::APSInt MCAInt = MCA->getValue()->EvaluateKnownConstInt(getContext());
Out << '{' << MCA->getSpelling() << ':' << MCAInt << '}';
Out << "{max_private_copies:" << MCAInt << '}';
Copy link
Contributor

Choose a reason for hiding this comment

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

If this change is required, then:

Suggested change
Out << "{max_private_copies:" << MCAInt << '}';
Out << "{private_copies:" << MCAInt << '}';

Copy link
Contributor

Choose a reason for hiding this comment

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

Ah, I got it. In that way we don't want to change LLVM IR output. It's looking hacky, isn't it? :)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Exactly.. As you mentioned, the "max" here is redundant, and is in fact misleading to customers. That's why we're getting rid of it. However, I'm keeping the IR the same for now because otherwise, we would have to change the llvm-spirv translator and the SPIRV spec to do things the right way. I should open a JIRA to track that effort, but this does not affect the customer flow.

Copy link
Contributor

@AGindinson AGindinson Jan 13, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

However, I'm keeping the IR the same for now because otherwise, we would have to change the llvm-spirv translator and the SPIRV spec to do things the right way.

I'd argue that no changes would have to be made to the SPIR-V specification, as it doesn't specify the LLVM IR layout. Would it be better to accompany this patch with a correspondent adjustment to the translator? I believe the fix would be trivial.

Copy link
Contributor

Choose a reason for hiding this comment

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

@mohammadfawaz please address @AGindinson comment. I'm OK to go either way.

Copy link
Contributor Author

@mohammadfawaz mohammadfawaz Jan 16, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

@MrSidims @AGindinson I will address the translator changes asap!

}
if (const auto *NBA = D->getAttr<IntelFPGANumBanksAttr>()) {
llvm::APSInt NBAInt = NBA->getValue()->EvaluateKnownConstInt(getContext());
Expand Down
14 changes: 7 additions & 7 deletions clang/lib/Sema/SemaDeclAttr.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3877,7 +3877,7 @@ void Sema::AddOneConstantValueAttr(Decl *D, const AttributeCommonInfo &CI,
E = ICE.get();
}

if (IntelFPGAMaxPrivateCopiesAttr::classof(&TmpAttr)) {
if (IntelFPGAPrivateCopiesAttr::classof(&TmpAttr)) {
if (!D->hasAttr<IntelFPGAMemoryAttr>())
D->addAttr(IntelFPGAMemoryAttr::CreateImplicit(
Context, IntelFPGAMemoryAttr::Default));
Expand Down Expand Up @@ -5243,7 +5243,7 @@ static bool checkIntelFPGARegisterAttrCompatibility(Sema &S, Decl *D,
InCompat = true;
if (checkAttrMutualExclusion<IntelFPGABankWidthAttr>(S, D, Attr))
InCompat = true;
if (checkAttrMutualExclusion<IntelFPGAMaxPrivateCopiesAttr>(S, D, Attr))
if (checkAttrMutualExclusion<IntelFPGAPrivateCopiesAttr>(S, D, Attr))
InCompat = true;
if (auto *NBA = D->getAttr<IntelFPGANumBanksAttr>())
if (!NBA->isImplicit() &&
Expand Down Expand Up @@ -5447,17 +5447,17 @@ void Sema::AddIntelFPGABankBitsAttr(Decl *D, const AttributeCommonInfo &CI,
IntelFPGABankBitsAttr(Context, CI, Args.data(), Args.size()));
}

static void handleIntelFPGAMaxPrivateCopiesAttr(Sema &S, Decl *D,
static void handleIntelFPGAPrivateCopiesAttr(Sema &S, Decl *D,
const ParsedAttr &Attr) {

if (S.LangOpts.SYCLIsHost)
return;

checkForDuplicateAttribute<IntelFPGAMaxPrivateCopiesAttr>(S, D, Attr);
checkForDuplicateAttribute<IntelFPGAPrivateCopiesAttr>(S, D, Attr);
if (checkAttrMutualExclusion<IntelFPGARegisterAttr>(S, D, Attr))
return;

S.AddOneConstantValueAttr<IntelFPGAMaxPrivateCopiesAttr>(
S.AddOneConstantValueAttr<IntelFPGAPrivateCopiesAttr>(
D, Attr, Attr.getArgAsExpr(0));
}

Expand Down Expand Up @@ -7965,8 +7965,8 @@ static void ProcessDeclAttribute(Sema &S, Scope *scope, Decl *D,
case ParsedAttr::AT_IntelFPGANumBanks:
handleOneConstantPowerTwoValueAttr<IntelFPGANumBanksAttr>(S, D, AL);
break;
case ParsedAttr::AT_IntelFPGAMaxPrivateCopies:
handleIntelFPGAMaxPrivateCopiesAttr(S, D, AL);
case ParsedAttr::AT_IntelFPGAPrivateCopies:
handleIntelFPGAPrivateCopiesAttr(S, D, AL);
break;
case ParsedAttr::AT_IntelFPGAMaxReplicates:
handleIntelFPGAMaxReplicatesAttr(S, D, AL);
Expand Down
4 changes: 2 additions & 2 deletions clang/test/CodeGenSYCL/intel-fpga-local.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -56,7 +56,7 @@ struct foo_two {
int f2 [[intelfpga::register]];
int f3 [[intelfpga::memory]];
int f4 [[intelfpga::bankwidth(4)]];
int f5 [[intelfpga::max_private_copies(8)]];
int f5 [[intelfpga::private_copies(8)]];
int f6 [[intelfpga::singlepump]];
int f7 [[intelfpga::doublepump]];
int f8 [[intelfpga::merge("foo", "depth")]];
Expand Down Expand Up @@ -130,7 +130,7 @@ void baz() {
// CHECK-DEVICE: %[[V_SEVEN:[0-9]+]] = bitcast{{.*}}v_seven
// CHECK-DEVICE: %[[V_SEVEN1:v_seven[0-9]+]] = bitcast{{.*}}v_seven
// CHECK-DEVICE: llvm.var.annotation{{.*}}%[[V_SEVEN1]],{{.*}}[[ANN9]]
int v_seven [[intelfpga::max_private_copies(4)]];
int v_seven [[intelfpga::private_copies(4)]];
// CHECK-DEVICE: %[[V_EIGHT:[0-9]+]] = bitcast{{.*}}v_eight
// CHECK-DEVICE: %[[V_EIGHT1:v_eight[0-9]+]] = bitcast{{.*}}v_eight
// CHECK-DEVICE: llvm.var.annotation{{.*}}%[[V_EIGHT1]],{{.*}}[[ANN10]]
Expand Down
46 changes: 23 additions & 23 deletions clang/test/SemaSYCL/intel-fpga-local.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -45,10 +45,10 @@ void foo1()

//CHECK: VarDecl{{.*}}v_seven
//CHECK: IntelFPGAMemoryAttr{{.*}}Implicit
//CHECK: IntelFPGAMaxPrivateCopiesAttr
//CHECK: IntelFPGAPrivateCopiesAttr
//CHECK-NEXT: ConstantExpr
//CHECK-NEXT: IntegerLiteral{{.*}}8{{$}}
[[intelfpga::max_private_copies(8)]] unsigned int v_seven[64];
[[intelfpga::private_copies(8)]] unsigned int v_seven[64];

//CHECK: VarDecl{{.*}}v_ten
//CHECK: IntelFPGAMemoryAttr{{.*}}Implicit
Expand Down Expand Up @@ -202,7 +202,7 @@ void foo1()

//expected-error@+2{{attributes are not compatible}}
[[intelfpga::register]]
[[intelfpga::max_private_copies(16)]]
[[intelfpga::private_copies(16)]]
//expected-note@-2 {{conflicting attribute is here}}
unsigned int reg_six_two[64];

Expand Down Expand Up @@ -304,37 +304,37 @@ void foo1()
unsigned int bw_seven[64];


// max_private_copies_
// private_copies_
//expected-error@+2{{attributes are not compatible}}
[[intelfpga::max_private_copies(16)]]
[[intelfpga::private_copies(16)]]
[[intelfpga::register]]
//expected-note@-2 {{conflicting attribute is here}}
unsigned int mc_one[64];

//CHECK: VarDecl{{.*}}mc_two
//CHECK: IntelFPGAMaxPrivateCopiesAttr
//CHECK: IntelFPGAPrivateCopiesAttr
//CHECK-NEXT: ConstantExpr
//CHECK-NEXT: IntegerLiteral{{.*}}8{{$}}
//CHECK: IntelFPGAMaxPrivateCopiesAttr
//CHECK: IntelFPGAPrivateCopiesAttr
//CHECK-NEXT: ConstantExpr
//CHECK-NEXT: IntegerLiteral{{.*}}16{{$}}
//expected-warning@+2{{is already applied}}
[[intelfpga::max_private_copies(8)]]
[[intelfpga::max_private_copies(16)]]
[[intelfpga::private_copies(8)]]
[[intelfpga::private_copies(16)]]
unsigned int mc_two[64];

//expected-error@+1{{'max_private_copies' attribute requires integer constant between 0 and 1048576 inclusive}}
[[intelfpga::max_private_copies(-4)]]
//expected-error@+1{{'private_copies' attribute requires integer constant between 0 and 1048576 inclusive}}
[[intelfpga::private_copies(-4)]]
unsigned int mc_four[64];

int i_max_private_copies = 32; // expected-note {{declared here}}
int i_private_copies = 32; // expected-note {{declared here}}
//expected-error@+1{{expression is not an integral constant expression}}
[[intelfpga::max_private_copies(i_max_private_copies)]]
//expected-note@-1{{read of non-const variable 'i_max_private_copies' is not allowed in a constant expression}}
[[intelfpga::private_copies(i_private_copies)]]
//expected-note@-1{{read of non-const variable 'i_private_copies' is not allowed in a constant expression}}
unsigned int mc_five[64];

//expected-error@+1{{'max_private_copies' attribute takes one argument}}
[[intelfpga::max_private_copies(4,8)]]
//expected-error@+1{{'private_copies' attribute takes one argument}}
[[intelfpga::private_copies(4,8)]]
unsigned int mc_six[64];

// numbanks
Expand Down Expand Up @@ -476,8 +476,8 @@ void foo1()
//expected-warning@+1{{unknown attribute '__doublepump__' ignored}}
unsigned int __attribute__((__doublepump__)) a_six;

//expected-warning@+1{{unknown attribute '__max_private_copies__' ignored}}
int __attribute__((__max_private_copies__(4))) a_seven;
//expected-warning@+1{{unknown attribute '__private_copies__' ignored}}
int __attribute__((__private_copies__(4))) a_seven;

//expected-warning@+1{{unknown attribute '__merge__' ignored}}
int __attribute__((__merge__("mrg1","depth"))) a_eight;
Expand All @@ -493,17 +493,17 @@ void foo1()
}

//expected-error@+1{{attribute only applies to local non-const variables and non-static data members}}
[[intelfpga::max_private_copies(8)]]
[[intelfpga::private_copies(8)]]
__attribute__((opencl_constant)) unsigned int ext_two[64] = { 1, 2, 3 };

void other2()
{
//expected-error@+1{{attribute only applies to local non-const variables and non-static data members}}
[[intelfpga::max_private_copies(8)]] const int ext_six[64] = { 0, 1 };
[[intelfpga::private_copies(8)]] const int ext_six[64] = { 0, 1 };
}

//expected-error@+1{{attribute only applies to local non-const variables and non-static data members}}
void other3([[intelfpga::max_private_copies(8)]] int pfoo) {}
void other3([[intelfpga::private_copies(8)]] int pfoo) {}

struct foo {
//CHECK: FieldDecl{{.*}}v_one
Expand Down Expand Up @@ -554,10 +554,10 @@ struct foo {

//CHECK: FieldDecl{{.*}}v_seven
//CHECK: IntelFPGAMemoryAttr{{.*}}Implicit
//CHECK: IntelFPGAMaxPrivateCopiesAttr
//CHECK: IntelFPGAPrivateCopiesAttr
//CHECK-NEXT: ConstantExpr
//CHECK-NEXT: IntegerLiteral{{.*}}4{{$}}
[[intelfpga::max_private_copies(4)]] unsigned int v_seven[64];
[[intelfpga::private_copies(4)]] unsigned int v_seven[64];

//CHECK: FieldDecl{{.*}}v_ten
//CHECK: IntelFPGAMemoryAttr{{.*}}Implicit
Expand Down
4 changes: 2 additions & 2 deletions clang/test/SemaSYCL/spurious-host-warning.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -38,9 +38,9 @@ void foo()
[[intelfpga::numbanks(8)]] unsigned int v_six[32];

#ifndef SYCLHOST
// expected-warning@+2 {{'max_private_copies' attribute ignored}}
// expected-warning@+2 {{'private_copies' attribute ignored}}
#endif
[[intelfpga::max_private_copies(8)]] unsigned int v_seven[64];
[[intelfpga::private_copies(8)]] unsigned int v_seven[64];

#ifndef SYCLHOST
// expected-warning@+2 {{'merge' attribute ignored}}
Expand Down