Skip to content

[SYCL] Let tablegen handle mutually exclusive decl attrs #3507

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 1 commit into from
Apr 8, 2021
Merged

[SYCL] Let tablegen handle mutually exclusive decl attrs #3507

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
11 changes: 11 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 @@ -1949,6 +1949,8 @@ def SYCLIntelFPGADisableLoopPipelining : DeclOrStmtAttr {
let Documentation = [SYCLIntelFPGADisableLoopPipeliningAttrDocs];
let SupportsNonconformingLambdaSyntax = 1;
}
def : MutualExclusions<[SYCLIntelFPGAInitiationInterval,
SYCLIntelFPGADisableLoopPipelining]>;

def SYCLIntelFPGAMaxInterleaving : StmtAttr {
let Spellings = [CXX11<"intelfpga","max_interleaving">,
Expand Down Expand Up @@ -2060,6 +2062,8 @@ def IntelFPGARegister : Attr {
let LangOpts = [SYCLIsDevice, SilentlyIgnoreSYCLIsHost];
let Documentation = [IntelFPGARegisterAttrDocs];
}
def : MutualExclusions<[IntelFPGADoublePump, IntelFPGASinglePump,
IntelFPGARegister]>;

// One integral argument.
def IntelFPGABankWidth : Attr {
Expand All @@ -2071,6 +2075,7 @@ def IntelFPGABankWidth : Attr {
let LangOpts = [SYCLIsDevice, SilentlyIgnoreSYCLIsHost];
let Documentation = [IntelFPGABankWidthAttrDocs];
}
def : MutualExclusions<[IntelFPGARegister, IntelFPGABankWidth]>;

def IntelFPGANumBanks : Attr {
let Spellings = [CXX11<"intelfpga","numbanks">,
Expand All @@ -2090,6 +2095,7 @@ def IntelFPGAPrivateCopies : InheritableAttr {
let Subjects = SubjectList<[IntelFPGALocalNonConstVar, Field], ErrorDiag>;
let Documentation = [IntelFPGAPrivateCopiesAttrDocs];
}
def : MutualExclusions<[IntelFPGARegister, IntelFPGAPrivateCopies]>;

// Two string arguments.
def IntelFPGAMerge : Attr {
Expand All @@ -2101,6 +2107,7 @@ def IntelFPGAMerge : Attr {
let LangOpts = [SYCLIsDevice, SilentlyIgnoreSYCLIsHost];
let Documentation = [IntelFPGAMergeAttrDocs];
}
def : MutualExclusions<[IntelFPGARegister, IntelFPGAMerge]>;

def IntelFPGAMaxReplicates : InheritableAttr {
let Spellings = [CXX11<"intelfpga","max_replicates">,
Expand All @@ -2111,6 +2118,7 @@ def IntelFPGAMaxReplicates : InheritableAttr {
let LangOpts = [SYCLIsDevice, SilentlyIgnoreSYCLIsHost];
let Documentation = [IntelFPGAMaxReplicatesAttrDocs];
}
def : MutualExclusions<[IntelFPGARegister, IntelFPGAMaxReplicates]>;

def IntelFPGASimpleDualPort : Attr {
let Spellings = [CXX11<"intelfpga","simple_dual_port">,
Expand All @@ -2120,6 +2128,7 @@ def IntelFPGASimpleDualPort : Attr {
let LangOpts = [SYCLIsDevice, SilentlyIgnoreSYCLIsHost];
let Documentation = [IntelFPGASimpleDualPortAttrDocs];
}
def : MutualExclusions<[IntelFPGARegister, IntelFPGASimpleDualPort]>;

def SYCLFPGAPipe : TypeAttr {
let Spellings = [GNU<"pipe">];
Expand All @@ -2146,6 +2155,7 @@ def IntelFPGABankBits : Attr {
let LangOpts = [SYCLIsDevice, SYCLIsHost];
let Documentation = [IntelFPGABankBitsDocs];
}
def : MutualExclusions<[IntelFPGARegister, IntelFPGABankBits]>;

def IntelFPGAForcePow2Depth : InheritableAttr {
let Spellings = [CXX11<"intelfpga","force_pow2_depth">,
Expand All @@ -2156,6 +2166,7 @@ def IntelFPGAForcePow2Depth : InheritableAttr {
let LangOpts = [SYCLIsDevice, SilentlyIgnoreSYCLIsHost];
let Documentation = [IntelFPGAForcePow2DepthAttrDocs];
}
def : MutualExclusions<[IntelFPGARegister, IntelFPGAForcePow2Depth]>;

def Naked : InheritableAttr {
let Spellings = [GCC<"naked">, Declspec<"naked">];
Expand Down
87 changes: 5 additions & 82 deletions clang/lib/Sema/SemaDeclAttr.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3307,12 +3307,6 @@ static void handleUseStallEnableClustersAttr(Sema &S, Decl *D,
static void handleSYCLIntelFPGADisableLoopPipeliningAttr(Sema &S, Decl *D,
const ParsedAttr &A) {
S.CheckDeprecatedSYCLAttributeSpelling(A);

// [[intel::disable_loop_pipelining] and [[intel::initiation_interval()]]
// attributes are incompatible.
if (checkAttrMutualExclusion<SYCLIntelFPGAInitiationIntervalAttr>(S, D, A))
return;

D->addAttr(::new (S.Context)
SYCLIntelFPGADisableLoopPipeliningAttr(S.Context, A));
}
Expand Down Expand Up @@ -3355,12 +3349,6 @@ void Sema::AddSYCLIntelFPGAInitiationIntervalAttr(Decl *D,
}
}

// [[intel::disable_loop_pipelining] and [[intel::initiation_interval()]]
// attributes are incompatible.
if (checkAttrMutualExclusion<SYCLIntelFPGADisableLoopPipeliningAttr>(*this, D,
CI))
return;

D->addAttr(::new (Context)
SYCLIntelFPGAInitiationIntervalAttr(Context, CI, E));
}
Expand All @@ -3385,12 +3373,6 @@ Sema::MergeSYCLIntelFPGAInitiationIntervalAttr(
}
}

// [[intel::initiation_interval()]] and [[intel::disable_loop_pipelining]
// attributes are incompatible.
if (checkAttrMutualExclusion<SYCLIntelFPGADisableLoopPipeliningAttr>(*this, D,
A))
return nullptr;

return ::new (Context)
SYCLIntelFPGAInitiationIntervalAttr(Context, A, A.getIntervalExpr());
}
Expand Down Expand Up @@ -5802,17 +5784,11 @@ static void handleSYCLIntelNoGlobalWorkOffsetAttr(Sema &S, Decl *D,
S.AddSYCLIntelNoGlobalWorkOffsetAttr(D, A, E);
}

/// Handle the [[intelfpga::doublepump]] and [[intelfpga::singlepump]] attributes.
/// One but not both can be specified
/// Both are incompatible with the __register__ attribute.
template <typename AttrType, typename IncompatAttrType>
/// Handle the [[intelfpga::doublepump]] and [[intelfpga::singlepump]]
/// attributes.
template <typename AttrType>
static void handleIntelFPGAPumpAttr(Sema &S, Decl *D, const ParsedAttr &A) {
checkForDuplicateAttribute<AttrType>(S, D, A);
if (checkAttrMutualExclusion<IncompatAttrType>(S, D, A))
return;

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

if (!D->hasAttr<IntelFPGAMemoryAttr>())
D->addAttr(IntelFPGAMemoryAttr::CreateImplicit(
Expand Down Expand Up @@ -5867,28 +5843,10 @@ static bool checkIntelFPGARegisterAttrCompatibility(Sema &S, Decl *D,
if (!MA->isImplicit() &&
checkAttrMutualExclusion<IntelFPGAMemoryAttr>(S, D, Attr))
InCompat = true;
if (checkAttrMutualExclusion<IntelFPGADoublePumpAttr>(S, D, Attr))
InCompat = true;
if (checkAttrMutualExclusion<IntelFPGASinglePumpAttr>(S, D, Attr))
InCompat = true;
if (checkAttrMutualExclusion<IntelFPGABankWidthAttr>(S, D, Attr))
InCompat = true;
if (checkAttrMutualExclusion<IntelFPGAPrivateCopiesAttr>(S, D, Attr))
InCompat = true;
if (auto *NBA = D->getAttr<IntelFPGANumBanksAttr>())
if (!NBA->isImplicit() &&
checkAttrMutualExclusion<IntelFPGANumBanksAttr>(S, D, Attr))
InCompat = true;
if (checkAttrMutualExclusion<IntelFPGAMaxReplicatesAttr>(S, D, Attr))
InCompat = true;
if (checkAttrMutualExclusion<IntelFPGASimpleDualPortAttr>(S, D, Attr))
InCompat = true;
if (checkAttrMutualExclusion<IntelFPGAMergeAttr>(S, D, Attr))
InCompat = true;
if (checkAttrMutualExclusion<IntelFPGABankBitsAttr>(S, D, Attr))
InCompat = true;
if (checkAttrMutualExclusion<IntelFPGAForcePow2DepthAttr>(S, D, Attr))
InCompat = true;

return InCompat;
}
Expand Down Expand Up @@ -5926,9 +5884,6 @@ static void handleIntelFPGASimpleDualPortAttr(Sema &S, Decl *D,
const ParsedAttr &AL) {
checkForDuplicateAttribute<IntelFPGASimpleDualPortAttr>(S, D, AL);

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

if (!D->hasAttr<IntelFPGAMemoryAttr>())
D->addAttr(IntelFPGAMemoryAttr::CreateImplicit(
S.Context, IntelFPGAMemoryAttr::Default));
Expand Down Expand Up @@ -5973,11 +5928,6 @@ void Sema::AddIntelFPGAMaxReplicatesAttr(Decl *D, const AttributeCommonInfo &CI,
}
}

// [[intel::fpga_register]] and [[intel::max_replicates()]]
// attributes are incompatible.
if (checkAttrMutualExclusion<IntelFPGARegisterAttr>(*this, D, CI))
return;

// If the declaration does not have an [[intel::fpga_memory]]
// attribute, this creates one as an implicit attribute.
if (!D->hasAttr<IntelFPGAMemoryAttr>())
Expand All @@ -6004,10 +5954,6 @@ Sema::MergeIntelFPGAMaxReplicatesAttr(Decl *D,
}
}
}
// [[intel::fpga_register]] and [[intel::max_replicates()]]
// attributes are incompatible.
if (checkAttrMutualExclusion<IntelFPGARegisterAttr>(*this, D, A))
return nullptr;

return ::new (Context) IntelFPGAMaxReplicatesAttr(Context, A, A.getValue());
}
Expand All @@ -6026,9 +5972,6 @@ static void handleIntelFPGAMaxReplicatesAttr(Sema &S, Decl *D,
static void handleIntelFPGAMergeAttr(Sema &S, Decl *D, const ParsedAttr &AL) {
checkForDuplicateAttribute<IntelFPGAMergeAttr>(S, D, AL);

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

SmallVector<StringRef, 2> Results;
for (int I = 0; I < 2; I++) {
StringRef Str;
Expand Down Expand Up @@ -6066,9 +6009,6 @@ static void handleIntelFPGABankBitsAttr(Sema &S, Decl *D, const ParsedAttr &A) {
if (!A.checkAtLeastNumArgs(S, 1))
return;

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

SmallVector<Expr *, 8> Args;
for (unsigned I = 0; I < A.getNumArgs(); ++I) {
Args.push_back(A.getArgAsExpr(I));
Expand Down Expand Up @@ -6176,11 +6116,6 @@ void Sema::AddIntelFPGAPrivateCopiesAttr(Decl *D, const AttributeCommonInfo &CI,
}
}

// [[intel::fpga_register]] and [[intel::private_copies()]]
// attributes are incompatible.
if (checkAttrMutualExclusion<IntelFPGARegisterAttr>(*this, D, CI))
return;

// If the declaration does not have [[intel::memory]]
// attribute, this creates default implicit memory.
if (!D->hasAttr<IntelFPGAMemoryAttr>())
Expand Down Expand Up @@ -6234,11 +6169,6 @@ void Sema::AddIntelFPGAForcePow2DepthAttr(Decl *D,
}
}

// [[intel::fpga_register]] and [[intel::force_pow2_depth()]]
// attributes are incompatible.
if (checkAttrMutualExclusion<IntelFPGARegisterAttr>(*this, D, CI))
return;

// If the declaration does not have an [[intel::fpga_memory]]
// attribute, this creates one as an implicit attribute.
if (!D->hasAttr<IntelFPGAMemoryAttr>())
Expand Down Expand Up @@ -6266,11 +6196,6 @@ Sema::MergeIntelFPGAForcePow2DepthAttr(Decl *D,
}
}

// [[intel::fpga_register]] and [[intel::force_pow2_depth()]]
// attributes are incompatible.
if (checkAttrMutualExclusion<IntelFPGARegisterAttr>(*this, D, A))
return nullptr;

return ::new (Context) IntelFPGAForcePow2DepthAttr(Context, A, A.getValue());
}

Expand Down Expand Up @@ -9571,12 +9496,10 @@ static void ProcessDeclAttribute(Sema &S, Scope *scope, Decl *D,

// Intel FPGA specific attributes
case ParsedAttr::AT_IntelFPGADoublePump:
handleIntelFPGAPumpAttr<IntelFPGADoublePumpAttr, IntelFPGASinglePumpAttr>(
S, D, AL);
handleIntelFPGAPumpAttr<IntelFPGADoublePumpAttr>(S, D, AL);
break;
case ParsedAttr::AT_IntelFPGASinglePump:
handleIntelFPGAPumpAttr<IntelFPGASinglePumpAttr, IntelFPGADoublePumpAttr>(
S, D, AL);
handleIntelFPGAPumpAttr<IntelFPGASinglePumpAttr>(S, D, AL);
break;
case ParsedAttr::AT_IntelFPGAMemory:
handleIntelFPGAMemoryAttr(S, D, AL);
Expand Down
4 changes: 2 additions & 2 deletions clang/test/SemaSYCL/initiation_interval.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -36,8 +36,8 @@
// expected-note@+1 {{conflicting attribute is here}}
[[intel::initiation_interval(4)]] [[intel::disable_loop_pipelining]] void func8();

// expected-error@+2 {{'initiation_interval' and 'disable_loop_pipelining' attributes are not compatible}}
// expected-note@+2 {{conflicting attribute is here}}
// expected-error@+3 {{'disable_loop_pipelining' and 'initiation_interval' attributes are not compatible}}
// expected-note@+1 {{conflicting attribute is here}}
[[intel::initiation_interval(4)]] void func9();
[[intel::disable_loop_pipelining]] void func9();

Expand Down
16 changes: 4 additions & 12 deletions clang/test/SemaSYCL/intel-fpga-global-const.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -24,12 +24,8 @@
//expected-note@-2{{previous attribute is here}}

// Merging of incompatible attributes.
// FIXME: Diagnostic order isn't correct, this isn't what we'd want here but
// this is an upstream issue. Merge function is calling here
// checkAttrMutualExclusion() function that has backwards diagnostic behavior.
// This should be fixed into upstream.
//expected-error@+2{{'max_replicates' and 'fpga_register' attributes are not compatible}}
//expected-note@+2{{conflicting attribute is here}}
//expected-error@+3{{'fpga_register' and 'max_replicates' attributes are not compatible}}
//expected-note@+1{{conflicting attribute is here}}
[[intel::max_replicates(12)]] extern const int var_max_replicates_2;
[[intel::fpga_register]] const int var_max_replicates_2 =0;

Expand Down Expand Up @@ -57,11 +53,7 @@
//expected-note@-2{{previous attribute is here}}

// Merging of incompatible attributes.
// FIXME: Diagnostic order isn't correct, this isn't what we'd want here but
// this is an upstream issue. Merge function is calling here
// checkAttrMutualExclusion() function that has backwards diagnostic behavior.
// This should be fixed into upstream.
//expected-error@+2{{'force_pow2_depth' and 'fpga_register' attributes are not compatible}}
//expected-note@+2{{conflicting attribute is here}}
//expected-error@+3{{'fpga_register' and 'force_pow2_depth' attributes are not compatible}}
//expected-note@+1{{conflicting attribute is here}}
[[intel::force_pow2_depth(1)]] extern const int var_force_pow2_depth_2;
[[intel::fpga_register]] const int var_force_pow2_depth_2 =0;
8 changes: 5 additions & 3 deletions clang/utils/TableGen/ClangAttrEmitter.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3664,10 +3664,12 @@ static void GenerateMutualExclusionsChecks(const Record &Attr,
if (Attr.isSubClassOf("TypeAttr"))
return;

// This means the attribute is either a statement attribute or a decl
// attribute, find out which.
// This means the attribute is either a statement attribute, a decl
// attribute, or both; find out which.
bool CurAttrIsStmtAttr =
Attr.isSubClassOf("StmtAttr") || Attr.isSubClassOf("DeclOrStmtAttr");
bool CurAttrIsDeclAttr =
!CurAttrIsStmtAttr || Attr.isSubClassOf("DeclOrStmtAttr");
Copy link
Contributor

@premanandrao premanandrao Apr 8, 2021

Choose a reason for hiding this comment

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

@AaronBallman, isn't the second bool the same as:
!Attr.isSubClassOf("StmtAttr") || Attr.isSubClassOf("DeclOrStmtAttr");

Wouldn't this match anything that is not StmtAttr?

Copy link
Contributor Author

@AaronBallman AaronBallman Apr 8, 2021

Choose a reason for hiding this comment

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

That's correct.

There are three kinds of attributes (four-ish if you count pragmas, but those never get here): type, stmt, and decl. We bail out early if it's a type attribute on line 3664, so by this point it should either be a statement attr, a decl attr, or both.

Copy link
Contributor

@premanandrao premanandrao Apr 8, 2021

Choose a reason for hiding this comment

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

Thanks!

Copy link
Contributor

@premanandrao premanandrao Apr 8, 2021

Choose a reason for hiding this comment

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

How do you want to handle this PR? Is there a separate PR to address statement attributes? I just don't want us to lose track of it. I will approve this one, assuming that one won't fall through the cracks.

Copy link
Contributor

@smanna12 smanna12 Apr 8, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

Is there a separate PR to address statement attributes?

I think so. Here is the discussion:
Originally posted by @AaronBallman in #3507 (comment)

Copy link
Contributor Author

@AaronBallman AaronBallman Apr 8, 2021

Choose a reason for hiding this comment

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

Statement attributes need more love; I'm working on that refactoring currently. If it looks like that refactoring has to be set down for some reason, I'll file an issue about it.

Copy link
Contributor

@bader bader Apr 8, 2021

Choose a reason for hiding this comment

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

@premanandrao, @AaronBallman, is this conversation resolved or I should wait for PR updates?

Copy link
Contributor Author

@AaronBallman AaronBallman Apr 8, 2021

Choose a reason for hiding this comment

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

I'm not expecting to change this PR further unless I've missed something or there are other concerns.

Copy link
Contributor

@premanandrao premanandrao Apr 8, 2021

Choose a reason for hiding this comment

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

@premanandrao, @AaronBallman, is this conversation resolved or I should wait for PR updates?

Sorry, I should have been more explicit. I considered this resolved.


std::vector<std::string> DeclAttrs, StmtAttrs;

Expand All @@ -3686,7 +3688,7 @@ static void GenerateMutualExclusionsChecks(const Record &Attr,

if (CurAttrIsStmtAttr)
StmtAttrs.push_back((AttrToExclude->getName() + "Attr").str());
else
if (CurAttrIsDeclAttr)
DeclAttrs.push_back((AttrToExclude->getName() + "Attr").str());
}
}
Expand Down