Skip to content

[SYCL][FPGA] Add Intel FPGA bank_bits attribute #876

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
Dec 5, 2019
Merged

[SYCL][FPGA] Add Intel FPGA bank_bits attribute #876

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
f6dd0ee
[SYCL] Add Intel FPGA bank_bits attribute
vmaksimo Nov 26, 2019
9fa3813
Update test
vmaksimo Nov 29, 2019
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
18 changes: 18 additions & 0 deletions clang/include/clang/Basic/Attr.td
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1744,6 +1744,24 @@ def SYCLFPGAPipe : TypeAttr {
let Documentation = [SYCLFPGAPipeDocs];
}

// Variadic integral arguments.
def IntelFPGABankBits : Attr {
let Spellings = [CXX11<"intelfpga", "bank_bits">];
let Args = [VariadicExprArgument<"Args">];
let Subjects = SubjectList<[IntelFPGAConstVar, IntelFPGALocalStaticSlaveMemVar,
Field], ErrorDiag>;
let LangOpts = [SYCLIsDevice, SYCLIsHost];
let Documentation = [IntelFPGABankBitsDocs];
let AdditionalMembers = [{
static unsigned getMinValue() {
return 0;
}
static unsigned getMaxValue() {
return 1024*1024;
Copy link
Contributor

Choose a reason for hiding this comment

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

IMO, this limit should be documented. The current documentation doesn't imply this value, or, for that matter, any particular number of bits.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

As I see, the limits for another attributes were not documented, so I just followed the common documenting style :)

}
}];
}

def Naked : InheritableAttr {
let Spellings = [GCC<"naked">, Declspec<"naked">];
let Subjects = SubjectList<[Function]>;
Expand Down
11 changes: 11 additions & 0 deletions clang/include/clang/Basic/AttrDocs.td
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1848,6 +1848,17 @@ loads).
}];
}

def IntelFPGABankBitsDocs : Documentation {
let Category = DocCatVariable;
let Heading = "bank_bits (IntelFPGA)";
let Content = [{
This attribute may be attached to a variable or struct member declaration and
instructs the backend to implement the variable or struct member in a banked
memory with 2^(N+1) banks, where the (b0, ..., bn) bits specified determine the
pointer address bits to bank on.
}];
}

def SYCLIntelKernelArgsRestrictDocs : Documentation {
let Category = DocCatVariable;
let Heading = "kernel_args_restrict";
Expand Down
4 changes: 4 additions & 0 deletions clang/include/clang/Basic/DiagnosticSemaKinds.td
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -139,6 +139,10 @@ def err_loop_unroll_compatibility : Error<
"incompatible loop unroll instructions: '%0' and '%1'">;
def err_pipe_attribute_arg_not_allowed : Error<
"'%0' mode for pipe attribute is not supported. Allowed modes: 'read_only', 'write_only'">;
def err_bankbits_numbanks_conflicting : Error<
"the number of bank_bits must be equal to ceil(log2(numbanks))">;
def err_bankbits_non_consecutive : Error<
"bank_bits must be consecutive">;

// C99 variable-length arrays
def ext_vla : Extension<"variable length arrays are a C99 feature">,
Expand Down
2 changes: 2 additions & 0 deletions clang/include/clang/Sema/Sema.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9245,6 +9245,8 @@ class Sema {
template <typename AttrType>
void AddOneConstantPowerTwoValueAttr(Decl *D, const AttributeCommonInfo &CI,
Expr *E);
void AddIntelFPGABankBitsAttr(Decl *D, const AttributeCommonInfo &CI,
Expr **Exprs, unsigned Size);

/// AddAlignedAttr - Adds an aligned attribute to a particular declaration.
void AddAlignedAttr(Decl *D, const AttributeCommonInfo &CI, Expr *E,
Expand Down
16 changes: 14 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 @@ -3966,8 +3966,20 @@ void CodeGenModule::generateIntelFPGAAnnotation(
Out << '{' << MCA->getSpelling() << ':' << MCAInt << '}';
}
if (const auto *NBA = D->getAttr<IntelFPGANumBanksAttr>()) {
llvm::APSInt BWAInt = NBA->getValue()->EvaluateKnownConstInt(getContext());
Out << '{' << NBA->getSpelling() << ':' << BWAInt << '}';
llvm::APSInt NBAInt = NBA->getValue()->EvaluateKnownConstInt(getContext());
Out << '{' << NBA->getSpelling() << ':' << NBAInt << '}';
}
if (const auto *BBA = D->getAttr<IntelFPGABankBitsAttr>()) {
Out << '{' << BBA->getSpelling() << ':';
for (IntelFPGABankBitsAttr::args_iterator I = BBA->args_begin(),
E = BBA->args_end();
I != E; ++I) {
if (I != BBA->args_begin())
Out << ',';
llvm::APSInt BBAInt = (*I)->EvaluateKnownConstInt(getContext());
Out << BBAInt;
}
Out << '}';
}
if (const auto *MRA = D->getAttr<IntelFPGAMaxReplicatesAttr>()) {
llvm::APSInt MRAInt = MRA->getValue()->EvaluateKnownConstInt(getContext());
Expand Down
100 changes: 100 additions & 0 deletions clang/lib/Sema/SemaDeclAttr.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3761,6 +3761,15 @@ void Sema::AddOneConstantPowerTwoValueAttr(Decl *D,
<< &TmpAttr;
return;
}
if (IntelFPGANumBanksAttr::classof(&TmpAttr)) {
if (auto *BBA = D->getAttr<IntelFPGABankBitsAttr>()) {
unsigned NumBankBits = BBA->args_size();
if (NumBankBits != Value.ceilLogBase2()) {
Diag(TmpAttr.getLocation(), diag::err_bankbits_numbanks_conflicting);
return;
}
}
}
E = ICE.get();
}

Expand Down Expand Up @@ -5105,6 +5114,8 @@ static bool checkIntelFPGARegisterAttrCompatibility(Sema &S, Decl *D,
InCompat = true;
if (checkAttrMutualExclusion<IntelFPGAMergeAttr>(S, D, Attr))
InCompat = true;
if (checkAttrMutualExclusion<IntelFPGABankBitsAttr>(S, D, Attr))
InCompat = true;

return InCompat;
}
Expand Down Expand Up @@ -5209,6 +5220,92 @@ static void handleIntelFPGAMergeAttr(Sema &S, Decl *D, const ParsedAttr &AL) {
IntelFPGAMergeAttr(S.Context, AL, Results[0], Results[1]));
}

/// Handle the bank_bits attribute.
/// This attribute accepts a list of values greater than zero.
/// This is incompatible with the register attribute.
/// The numbanks and bank_bits attributes are related. If numbanks exists
/// when handling bank_bits they are checked for consistency. If numbanks
/// hasn't been added yet an implicit one is added with the correct value.
/// If the user later adds a numbanks attribute the implicit one is removed.
/// The values must be consecutive values (i.e. 3,4,5 or 2,1).
static void handleIntelFPGABankBitsAttr(Sema &S, Decl *D,
const ParsedAttr &Attr) {
checkForDuplicateAttribute<IntelFPGABankBitsAttr>(S, D, Attr);
Copy link
Contributor

Choose a reason for hiding this comment

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

Add D decl validity check:
if (D->isInvalidDecl()) return;

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Are you sure we need this check? It seems that other attributes handling has no such check. I think that it is handled higher in the call stack.

Copy link
Contributor

Choose a reason for hiding this comment

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

It depends on a contract we have with clang code:

  1. We trust function inputs;
  2. We don't trust function inputs.

Copy link
Contributor

Choose a reason for hiding this comment

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

I don't have here a strong evidences to push anyone to follow my own preferences, but as far as I'm a pedantic person - I'd prefer to add this check at least in a code what I write.

Copy link
Contributor

Choose a reason for hiding this comment

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

Quickly looking into the code I did not find such checks.

Copy link
Contributor

Choose a reason for hiding this comment

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

I mean declaration invalidation checks before attribute handling earlier in the code.


if (!checkAttributeAtLeastNumArgs(S, Attr, 1))
return;

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

SmallVector<Expr *, 8> Args;
for (unsigned I = 0; I < Attr.getNumArgs(); ++I) {
Args.push_back(Attr.getArgAsExpr(I));
}

S.AddIntelFPGABankBitsAttr(D, Attr, Args.data(), Args.size());
}

void Sema::AddIntelFPGABankBitsAttr(Decl *D, const AttributeCommonInfo &CI,
Expr **Exprs, unsigned Size) {
IntelFPGABankBitsAttr TmpAttr(Context, CI, Exprs, Size);
SmallVector<Expr *, 8> Args;
SmallVector<int64_t, 8> Values;
bool ListIsValueDep = false;
for (auto *E : TmpAttr.args()) {
llvm::APSInt Value(32, /*IsUnsigned=*/false);
Expr::EvalResult Result;
ListIsValueDep = ListIsValueDep || E->isValueDependent();
if (!E->isValueDependent()) {
ExprResult ICE;
if (checkRangedIntegralArgument<IntelFPGABankBitsAttr>(E, &TmpAttr, ICE))
return;
if (E->EvaluateAsInt(Result, Context))
Value = Result.Val.getInt();
E = ICE.get();
}
Args.push_back(E);
Values.push_back(Value.getExtValue());
}

// Check that the list is consecutive.
if (!ListIsValueDep && Values.size() > 1) {
bool ListIsAscending = Values[0] < Values[1];
for (int I = 0, E = Values.size() - 1; I < E; ++I) {
if (Values[I + 1] != Values[I] + (ListIsAscending ? 1 : -1)) {
Diag(CI.getLoc(), diag::err_bankbits_non_consecutive) << &TmpAttr;
return;
}
}
}

// Check or add the related numbanks attribute.
if (auto *NBA = D->getAttr<IntelFPGANumBanksAttr>()) {
Expr *E = NBA->getValue();
if (!E->isValueDependent()) {
Expr::EvalResult Result;
E->EvaluateAsInt(Result, Context);
llvm::APSInt Value = Result.Val.getInt();
if (Args.size() != Value.ceilLogBase2()) {
Diag(TmpAttr.getLoc(), diag::err_bankbits_numbanks_conflicting);
return;
}
}
} else {
llvm::APInt Num(32, (unsigned)(1 << Args.size()));
Expr *NBE =
IntegerLiteral::Create(Context, Num, Context.IntTy, SourceLocation());
D->addAttr(IntelFPGANumBanksAttr::CreateImplicit(Context, NBE));
}

if (!D->hasAttr<IntelFPGAMemoryAttr>())
D->addAttr(IntelFPGAMemoryAttr::CreateImplicit(
Context, IntelFPGAMemoryAttr::Default));

D->addAttr(::new (Context)
IntelFPGABankBitsAttr(Context, CI, Args.data(), Args.size()));
}

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

Expand Down Expand Up @@ -7635,6 +7732,9 @@ static void ProcessDeclAttribute(Sema &S, Scope *scope, Decl *D,
case ParsedAttr::AT_IntelFPGAMerge:
handleIntelFPGAMergeAttr(S, D, AL);
break;
case ParsedAttr::AT_IntelFPGABankBits:
handleIntelFPGABankBitsAttr(S, D, AL);
break;

case ParsedAttr::AT_AnyX86NoCallerSavedRegisters:
handleSimpleAttribute<AnyX86NoCallerSavedRegistersAttr>(S, D, AL);
Expand Down
31 changes: 29 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 @@ -18,12 +18,15 @@
//CHECK: [[ANN16:@.str.[0-9]*]] = {{.*}}foobar
//CHECK: [[ANN17:@.str.[0-9]*]] = {{.*}}{memory:MLAB}{sizeinfo:4,500}
//CHECK: [[ANN18:@.str.[0-9]*]] = {{.*}}{memory:BLOCK_RAM}{sizeinfo:4,10,2}
//CHECK: [[ANN19:@.str.[0-9]*]] = {{.*}}{memory:DEFAULT}{sizeinfo:4}{numbanks:4}{bank_bits:4,5}
//CHECK: [[ANN20:@.str.[0-9]*]] = {{.*}}{memory:DEFAULT}{sizeinfo:4,10,2}{bankwidth:16}{numbanks:2}{bank_bits:0}
//CHECK: [[ANN21:@.str.[0-9]*]] = {{.*}}{memory:MLAB}{sizeinfo:4}{numbanks:8}{bank_bits:5,4,3}

//CHECK: @llvm.global.annotations
//CHECK-SAME: { i8 addrspace(1)* bitcast (i32 addrspace(1)* @_ZZ3quxiE5a_one to i8 addrspace(1)*)
//CHECK-SAME: [[ANN1]]{{.*}}i32 154
//CHECK-SAME: [[ANN1]]{{.*}}i32 157
//CHECK-SAME: { i8 addrspace(1)* bitcast (i32 addrspace(1)* @_ZZ3quxiE5b_two to i8 addrspace(1)*)
//CHECK-SAME: [[ANN16]]{{.*}}i32 158
//CHECK-SAME: [[ANN16]]{{.*}}i32 161

void foo() {
//CHECK: %[[VAR_ONE:[0-9]+]] = bitcast{{.*}}var_one
Expand Down Expand Up @@ -172,6 +175,29 @@ void size_info() {
[[intelfpga::memory("BLOCK_RAM")]] int var_b[10][2];
}

struct s {
int a [[intelfpga::bank_bits(4, 5)]];
};

void bankbits() {
//CHECK: %[[VAR:[0-9]+]] = bitcast{{.*}}%a
//CHECK: %[[VAR1:a[0-9]+]] = bitcast{{.*}}%a
//CHECK: @llvm.var.annotation{{.*}}%[[VAR1]],{{.*}}[[ANN19]]
[[intelfpga::bank_bits(4,5)]] int a;
//CHECK: %[[VARB:[0-9]+]] = bitcast{{.*}}%b
//CHECK: %[[VARB1:b[0-9]+]] = bitcast{{.*}}%b
//CHECK: @llvm.var.annotation{{.*}}%[[VARB1]],{{.*}}[[ANN20]]
[[intelfpga::bank_bits(0), intelfpga::bankwidth(16)]] int b[10][2];
//CHECK: %[[VARC:[0-9]+]] = bitcast{{.*}}%c
//CHECK: %[[VARC1:c[0-9]+]] = bitcast{{.*}}%c
//CHECK: @llvm.var.annotation{{.*}}%[[VARC1]],{{.*}}[[ANN21]]
[[intelfpga::bank_bits(5,4,3), intelfpga::numbanks(8), intelfpga::memory("MLAB")]] int c;
struct s s2;
//CHECK: %[[FIELD_A:.*]] = getelementptr inbounds %struct.{{.*}}.s{{.*}}
//CHECK: call i32* @llvm.ptr.annotation.p0i32{{.*}}%[[FIELD_A]]{{.*}}[[ANN19]]
s2.a = 0;
}

void field_addrspace_cast() {
struct state {
[[intelfpga::numbanks(2)]] int mem[8];
Expand Down Expand Up @@ -204,6 +230,7 @@ int main() {
baz();
qux(42);
size_info();
bankbits();
field_addrspace_cast();
});
return 0;
Expand Down
Loading