[SYCL] Store expression as ConstantExpr in Semantic Attribute #3169

elizabethandrews · 2021-02-05T18:20:36Z

Currently , the behavior is inconsistent for FPGA attributes. We evaluate and store the ConstantExpr for some SYCL/FPGA attributes , and discard it for others. This patch is part 1 of making the behavior consistent. This patch saves the ConstantExpr for the following attributes. The ConstantExpr is then extracted from the Sematic attributes and used in CodeGen instead of reevaluating it.

SYCLIntelNoGlobalWorkOffsetAttr,
SYCLIntelMaxGlobalWorkDimAttr,
SYCLIntelNumSimdWorkItemsAttr ,
IntelReqdSubGroupSizeAttr ,
SYCLIntelSchedulerTargetFmaxMhzAttr ,
ReqdWorkGroupSizeAttr,
IntelReqdSubGroupSizeAttr,
SYCLIntelMaxWorkGroupSizeAttr

A consequence of this change is a change in diagnostic when the attribute argument is not an integer constant expression. The new diagnostic is an existing llvm-project diagnostic.

A follow-up patch(s) will address the remaining attributes.

Signed-off-by: Elizabeth Andrews [email protected]

Currently , the behavior is inconsistent for FPGA attributes. We store the ConstantExpr for some SYCL/FPGA attributes (not all). This patch is part 1 of making it consistent. It modfies addIntelSYCLSingleArgFunctionAttr() to save ConstantExpr. This saves ConstantExpr for all attributes that calls this function. A follow-up patch will address the remaining attributes. Signed-off-by: Elizabeth Andrews <[email protected]>

AaronBallman

The changes look reasonable, but there are a bit more gains to be had from the change. For instance, in:

llvm/clang/lib/CodeGen/CodeGenFunction.cpp

Line 726 in 4fed824

Optional<llvm::APSInt> ArgVal =

You can start to take advantage of the fact that we don't need to recompute the constant expression -- we can just pull it back out of the attribute and get the value.

Do you think those sort of changes should be part of this patch, or should they be handled in a separate patch? My thinking is they'd be reasonable in this patch, but you might have other plans.

smanna12 · 2021-02-06T01:26:12Z

one comment here: SYCLIntelSchedulerTargetFmaxMhzAttr currently uses AddOneConstantValueAttr() instead of addIntelSYCLSingleArgFunctionAttr(), So this attribute already supports ConstantExpr. Please remove this attribute from the PR description.

elizabethandrews · 2021-02-06T11:28:41Z

one comment here: SYCLIntelSchedulerTargetFmaxMhzAttr currently uses AddOneConstantValueAttr() instead of addIntelSYCLSingleArgFunctionAttr(), So this attribute already supports ConstantExpr. Please remove this attribute from the PR description.

There is some inconsistency with SYCLIntelSchedulerTargetFmaxMhzAttr . It uses addIntelSYCLSingleArgFunctionAttr() for template instantiation. So in this case constant expression was not being stored.

Signed-off-by: Elizabeth Andrews <[email protected]>

elizabethandrews · 2021-02-08T12:53:12Z

The changes look reasonable, but there are a bit more gains to be had from the change. For instance, in:

llvm/clang/lib/CodeGen/CodeGenFunction.cpp

Line 726 in 4fed824

Optional<llvm::APSInt> ArgVal =

You can start to take advantage of the fact that we don't need to recompute the constant expression -- we can just pull it back out of the attribute and get the value.

Do you think those sort of changes should be part of this patch, or should they be handled in a separate patch? My thinking is they'd be reasonable in this patch, but you might have other plans.

I'm working on this. The change I made in CodeGen caused compiler to crash for tests. The system I work on has been very slow today. So I'm building on another system to debug the crash.

Signed-off-by: Elizabeth Andrews <[email protected]>

elizabethandrews · 2021-02-09T12:23:37Z

The changes look reasonable, but there are a bit more gains to be had from the change. For instance, in:

llvm/clang/lib/CodeGen/CodeGenFunction.cpp

Line 726 in 4fed824

Optional<llvm::APSInt> ArgVal =

You can start to take advantage of the fact that we don't need to recompute the constant expression -- we can just pull it back out of the attribute and get the value.
Do you think those sort of changes should be part of this patch, or should they be handled in a separate patch? My thinking is they'd be reasonable in this patch, but you might have other plans.

I'm working on this. The change I made in CodeGen caused compiler to crash for tests. The system I work on has been very slow today. So I'm building on another system to debug the crash.

This is done now. The following code (in CodeGen) in my original patch (not pushed to Github) crashed when handling an attribute which used common Sema handler for openCL and C++ attribute.

const auto *Arg = dyn_cast<ConstantExpr>(A->getValue());
Optional<llvm::APSInt> ArgVal = Arg->getAPValueResult().getInt();

When the ConstantExpr is generated, openCL attributes creates it with ResultStorageKind = RSK_None (https://clang.llvm.org/doxygen/SemaExpr_8cpp_source.html#l16135), which in turn leads to a crash in getInt() for an isInt() check.

So I changed the code to use EvaluateAsInt() which I believe does not evaluate the constant expression again. @AaronBallman does this sound correct to you? I am not very sure if what I did is right.

AaronBallman · 2021-02-09T12:25:54Z

clang/include/clang/Sema/Sema.h

+  if (!XDimExpr->isValueDependent() && !YDimExpr->isValueDependent() &&
+      !ZDimExpr->isValueDependent()) {
+
+    if (!handleMaxWorkSizeAttrExpr(*this, CI, XDimExpr))


I think it might be more clear if we make the pattern:

XDimExpr = checkMaxWorkSizeAttrExpr(*this, CI, XDimExpr); YDimExpr = checkMaxWorkSizeAttrExpr(*this, CI, YDimExpr); ZDimExpr = checkMaxWorkSizeAttrExpr(*this, CI, ZDimExpr); if (!XDimExpr || !YDimExpr || !ZDimExpr) return;

to make it super obvious that we're intentionally replacing the expression we pulled from the parsed attribute.

AaronBallman · 2021-02-09T12:29:19Z

clang/lib/CodeGen/CodeGenFunction.cpp

-    assert(ArgVal.hasValue() && "Not an integer constant expression");
+
+    Expr::EvalResult Result;
+    assert(A->getValue()->EvaluateAsInt(Result, getContext()) &&


Result will always be uninitialized when assertions are disabled. I think a better approach would be:

const auto *CE = dyn_cast<ConstantExpr>(A->getValue()); assert(CE && "Not an integer constant expression"); llvm::APSInt ArgVal = CE->getResultAsAPSInt();

This same comment applies in all the cases.

As an aside: if our CI pipeline doesn't notice the failure caused by the use of assert here, we may be missing some CI coverage for a release without asserts build.

Oh thanks for catching that! I did not think of that. Will make the change now

I just noticed you suggested getResultAsAPSInt(). This crashes for the same reason under discussion here - #3169 (comment).

AaronBallman · 2021-02-09T12:45:52Z

The changes look reasonable, but there are a bit more gains to be had from the change. For instance, in:

llvm/clang/lib/CodeGen/CodeGenFunction.cpp

Line 726 in 4fed824

Optional<llvm::APSInt> ArgVal =

You can start to take advantage of the fact that we don't need to recompute the constant expression -- we can just pull it back out of the attribute and get the value.
Do you think those sort of changes should be part of this patch, or should they be handled in a separate patch? My thinking is they'd be reasonable in this patch, but you might have other plans.

I'm working on this. The change I made in CodeGen caused compiler to crash for tests. The system I work on has been very slow today. So I'm building on another system to debug the crash.

This is done now. The following code (in CodeGen) in my original patch (not pushed to Github) crashed when handling an attribute which used common Sema handler for openCL and C++ attribute.
const auto *Arg = dyn_cast<ConstantExpr>(A->getValue());
Optional<llvm::APSInt> ArgVal = Arg->getAPValueResult().getInt();
When the ConstantExpr is generated, openCL attributes creates it with ResultStorageKind = RSK_None (https://clang.llvm.org/doxygen/SemaExpr_8cpp_source.html#l16135), which in turn leads to a crash in getInt() for an isInt() check.

So I changed the code to use EvaluateAsInt() which I believe does not evaluate the constant expression again. @AaronBallman does this sound correct to you? I am not very sure if what I did is right.

What the heck is going on with that code in SemaExpr.cpp? It's figured out that it has a constant expression and it knows what APSInt value is generated from it, but it makes a ConstantExpr that doesn't hold the value. That seems odd to me and I sort of think https://clang.llvm.org/doxygen/SemaExpr_8cpp_source.html#l16141 should be retaining the value if we stored one into *Result. e.g.,

if (!isa<ConstantExpr>(E))
  E = Result ? ConstantExpr::Create(Context, E, APValue(*Result)) : ConstantExpr::Create(Context, E);

@erichkeane, do you have thoughts on this?

erichkeane · 2021-02-09T13:57:39Z

The changes look reasonable, but there are a bit more gains to be had from the change. For instance, in:

llvm/clang/lib/CodeGen/CodeGenFunction.cpp

Line 726 in 4fed824

Optional<llvm::APSInt> ArgVal =

You can start to take advantage of the fact that we don't need to recompute the constant expression -- we can just pull it back out of the attribute and get the value.
Do you think those sort of changes should be part of this patch, or should they be handled in a separate patch? My thinking is they'd be reasonable in this patch, but you might have other plans.

I'm working on this. The change I made in CodeGen caused compiler to crash for tests. The system I work on has been very slow today. So I'm building on another system to debug the crash.

This is done now. The following code (in CodeGen) in my original patch (not pushed to Github) crashed when handling an attribute which used common Sema handler for openCL and C++ attribute.
const auto *Arg = dyn_cast<ConstantExpr>(A->getValue());
Optional<llvm::APSInt> ArgVal = Arg->getAPValueResult().getInt();
When the ConstantExpr is generated, openCL attributes creates it with ResultStorageKind = RSK_None (https://clang.llvm.org/doxygen/SemaExpr_8cpp_source.html#l16135), which in turn leads to a crash in getInt() for an isInt() check.
So I changed the code to use EvaluateAsInt() which I believe does not evaluate the constant expression again. @AaronBallman does this sound correct to you? I am not very sure if what I did is right.
What the heck is going on with that code in SemaExpr.cpp? It's figured out that it has a constant expression and it knows what APSInt value is generated from it, but it makes a ConstantExpr that doesn't hold the value. That seems odd to me and I sort of think https://clang.llvm.org/doxygen/SemaExpr_8cpp_source.html#l16141 should be retaining the value if we stored one into *Result. e.g.,
if (!isa<ConstantExpr>(E))
  E = Result ? ConstantExpr::Create(Context, E, APValue(*Result)) : ConstantExpr::Create(Context, E);
@erichkeane, do you have thoughts on this?

Woah, yeah, that is pretty strange. I'm guessing this causes us to have to re-evaluate it later, but I don't know what the symptoms of that would be.

AaronBallman · 2021-02-09T14:10:26Z

@erichkeane, do you have thoughts on this?

Woah, yeah, that is pretty strange. I'm guessing this causes us to have to re-evaluate it later, but I don't know what the symptoms of that would be.

The impetus behind this patch is that I would like the semantic attribute to store a ConstantExpr rather than an Expr so that we can evaluate the constant expression once (when converting the parsed attribute into a semantic attribute, potentially at template instantiation time if the expr is value-dependent), and then later we can just pull the ConstantExpr out of the semantic attribute and interrogate it directly for its value. I thought this was a reasonable approach, but Elizabeth found this particular scenario where the constant evaluator produces a ConstantExpr that has no value associated with it.

So I guess the question really boils down to: what should we do about it?

I think it might be interesting to try making the change to SemaExpr.cpp I suggested above to see if anything breaks or not (independent of this patch). If nothing breaks, perhaps that would be a patch worth floating by the community to see if Richard accepts it or explains what we've misunderstood?

An alternative is to use EvaluateAsInt() rather than presuming we can get the constant value out of the expression directly. My fear with that approach is that we wind up re-evaluating the constant expression, but I've not validated that fear by looking at the implementation yet.

erichkeane · 2021-02-09T14:17:24Z

@erichkeane, do you have thoughts on this?

Woah, yeah, that is pretty strange. I'm guessing this causes us to have to re-evaluate it later, but I don't know what the symptoms of that would be.

The impetus behind this patch is that I would like the semantic attribute to store a ConstantExpr rather than an Expr so that we can evaluate the constant expression once (when converting the parsed attribute into a semantic attribute, potentially at template instantiation time if the expr is value-dependent), and then later we can just pull the ConstantExpr out of the semantic attribute and interrogate it directly for its value. I thought this was a reasonable approach, but Elizabeth found this particular scenario where the constant evaluator produces a ConstantExpr that has no value associated with it.

So I guess the question really boils down to: what should we do about it?

I think it might be interesting to try making the change to SemaExpr.cpp I suggested above to see if anything breaks or not (independent of this patch). If nothing breaks, perhaps that would be a patch worth floating by the community to see if Richard accepts it or explains what we've misunderstood?

An alternative is to use EvaluateAsInt() rather than presuming we can get the constant value out of the expression directly. My fear with that approach is that we wind up re-evaluating the constant expression, but I've not validated that fear by looking at the implementation yet.

I'm pretty sure EvaluateAsInt does basically everything EvaluateAsIntegerConstant anyway, I think the idea of deciding that it is a ConstantExpr and storing that is a good idea.

Short term, we could just try setting the ConstantExpr's value to the result we find as a 'fixup' (ConstantExpr::SetResult), but I think changing the SemaExpr version is definitely worth an attempt.

elizabethandrews · 2021-02-09T14:56:52Z

@erichkeane, do you have thoughts on this?

Woah, yeah, that is pretty strange. I'm guessing this causes us to have to re-evaluate it later, but I don't know what the symptoms of that would be.

The impetus behind this patch is that I would like the semantic attribute to store a ConstantExpr rather than an Expr so that we can evaluate the constant expression once (when converting the parsed attribute into a semantic attribute, potentially at template instantiation time if the expr is value-dependent), and then later we can just pull the ConstantExpr out of the semantic attribute and interrogate it directly for its value. I thought this was a reasonable approach, but Elizabeth found this particular scenario where the constant evaluator produces a ConstantExpr that has no value associated with it.
So I guess the question really boils down to: what should we do about it?
I think it might be interesting to try making the change to SemaExpr.cpp I suggested above to see if anything breaks or not (independent of this patch). If nothing breaks, perhaps that would be a patch worth floating by the community to see if Richard accepts it or explains what we've misunderstood?
An alternative is to use EvaluateAsInt() rather than presuming we can get the constant value out of the expression directly. My fear with that approach is that we wind up re-evaluating the constant expression, but I've not validated that fear by looking at the implementation yet.

I'm pretty sure EvaluateAsInt does basically everything EvaluateAsIntegerConstant anyway, I think the idea of deciding that it is a ConstantExpr and storing that is a good idea.

Short term, we could just try setting the ConstantExpr's value to the result we find as a 'fixup' (ConstantExpr::SetResult), but I think changing the SemaExpr version is definitely worth an attempt.

I was mistaken in my earlier comment when I mentioned EvaluateAsInt does not evaluate the ConstantExpr again. I was confused with Create vs Evaluate. I took another look at the implementation and I think EvaluateAsInt is reevaluating the constant expression. However I do not fully understand the implementation at the moment, so I could be wrong.

I'll look into Erich's suggestion of using setResult()

keryell · 2021-02-10T03:22:54Z

@Ralender can you look at this, since you have worked on the constant evaluator...

elizabethandrews · 2021-02-10T16:22:35Z

I'll look into Erich's suggestion of using setResult()

This did not work. The ConstantExpr is created with ResultStorageKind = RSK_None. This leads to setResult(APValue, Context) asserting here - https://clang.llvm.org/doxygen/Expr_8cpp_source.html#l00329, because storage kind of value is RSK_Int64. This is what I tried in Sema.h

 CE = dyn_cast<ConstantExpr>(E);
 if (!CE->hasAPValueResult())
   CE->SetResult(APValue(ArgVal), Context);

Where ArgVal is of type APSInt.

Is this what you meant @erichkeane or am I doing something wrong?

erichkeane · 2021-02-10T16:26:59Z

I'll look into Erich's suggestion of using setResult()

This did not work. The ConstantExpr is created with ResultStorageKind = RSK_None. This leads to setResult(APValue, Context) asserting here - https://clang.llvm.org/doxygen/Expr_8cpp_source.html#l00329, because storage kind of value is RSK_Int64. This is what I tried in Sema.h
 CE = dyn_cast<ConstantExpr>(E);
 if (!CE->hasAPValueResult())
   CE->SetResult(APValue(ArgVal), Context);
Where ArgVal is of type APSInt.

Is this what you meant @erichkeane or am I doing something wrong?

No, thats what I meant... I hadn't realized that ConstantExpr used trailing storage to determine what is being stored, so the only way to do this would be to create a new ConstantExpr, or to try out @AaronBallman 's suggestion above of changing SemaExpr.cpp.

AaronBallman · 2021-02-10T16:28:15Z

I'll look into Erich's suggestion of using setResult()

This did not work. The ConstantExpr is created with ResultStorageKind = RSK_None. This leads to setResult(APValue, Context) asserting here - https://clang.llvm.org/doxygen/Expr_8cpp_source.html#l00329, because storage kind of value is RSK_Int64. This is what I tried in Sema.h
 CE = dyn_cast<ConstantExpr>(E);
 if (!CE->hasAPValueResult())
   CE->SetResult(APValue(ArgVal), Context);
Where ArgVal is of type APSInt.
Is this what you meant @erichkeane or am I doing something wrong?
No, thats what I meant... I hadn't realized that ConstantExpr used trailing storage to determine what is being stored, so the only way to do this would be to create a new ConstantExpr, or to try out @AaronBallman 's suggestion above of changing SemaExpr.cpp.

FWIW, I am trying my approach on a community build right now. I'll report back whether it causes lit tests to fail or not when I have results.

AaronBallman · 2021-02-10T20:10:29Z

I'll look into Erich's suggestion of using setResult()

This did not work. The ConstantExpr is created with ResultStorageKind = RSK_None. This leads to setResult(APValue, Context) asserting here - https://clang.llvm.org/doxygen/Expr_8cpp_source.html#l00329, because storage kind of value is RSK_Int64. This is what I tried in Sema.h
 CE = dyn_cast<ConstantExpr>(E);
 if (!CE->hasAPValueResult())
   CE->SetResult(APValue(ArgVal), Context);
Where ArgVal is of type APSInt.
Is this what you meant @erichkeane or am I doing something wrong?
No, thats what I meant... I hadn't realized that ConstantExpr used trailing storage to determine what is being stored, so the only way to do this would be to create a new ConstantExpr, or to try out @AaronBallman 's suggestion above of changing SemaExpr.cpp.
FWIW, I am trying my approach on a community build right now. I'll report back whether it causes lit tests to fail or not when I have results.

I get the following failures when testing on Windows with an MSVC 2019-built community clang with my suggested changes:

Failed Tests (30):
  Clang :: AST/ast-dump-c-attr.c
  Clang :: AST/ast-dump-decl-json.c
  Clang :: AST/ast-dump-decl.c
  Clang :: AST/ast-dump-openmp-distribute-parallel-for-simd.c
  Clang :: AST/ast-dump-openmp-distribute-parallel-for.c
  Clang :: AST/ast-dump-openmp-distribute-simd.c
  Clang :: AST/ast-dump-openmp-distribute.c
  Clang :: AST/ast-dump-openmp-for-simd.c
  Clang :: AST/ast-dump-openmp-for.c
  Clang :: AST/ast-dump-openmp-ordered.c
  Clang :: AST/ast-dump-openmp-parallel-for-simd.c
  Clang :: AST/ast-dump-openmp-parallel-for.c
  Clang :: AST/ast-dump-openmp-simd.c
  Clang :: AST/ast-dump-openmp-target-parallel-for-simd.c
  Clang :: AST/ast-dump-openmp-target-parallel-for.c
  Clang :: AST/ast-dump-openmp-target-simd.c
  Clang :: AST/ast-dump-openmp-target-teams-distribute-parallel-for-simd.c
  Clang :: AST/ast-dump-openmp-target-teams-distribute-parallel-for.c
  Clang :: AST/ast-dump-openmp-target-teams-distribute-simd.c
  Clang :: AST/ast-dump-openmp-target-teams-distribute.c
  Clang :: AST/ast-dump-openmp-taskloop-simd.c
  Clang :: AST/ast-dump-openmp-taskloop.c
  Clang :: AST/ast-dump-openmp-teams-distribute-parallel-for-simd.c
  Clang :: AST/ast-dump-openmp-teams-distribute-parallel-for.c
  Clang :: AST/ast-dump-openmp-teams-distribute-simd.c
  Clang :: AST/ast-dump-openmp-teams-distribute.c
  Clang :: AST/ast-dump-records.c
  Clang :: CodeGen/thinlto_embed_bitcode.ll
  Clang :: Frontend/embed-bitcode-noopt.c
  Clang :: Frontend/embed-bitcode-noopt.ll


Testing Time: 5421.01s
  Unsupported      :   835
  Passed           : 12504
  Expectedly Failed:    28
  Failed           :    30

The AST dumping tests are failing because some ConstantExpr nodes are now printing an IntegerLiteral where they were not previously doing so, but that behavior seems reasonable to me. The embedded bitcode test failures always happen for me (it's a permissions issue on Windows that I've never bothered to look into).

erichkeane · 2021-02-10T20:13:36Z

Yeah, I'd imagine ASTDump tests would fail, so that makes a lot of sense to me. I think that generally confirms to me this is the 'right' thing to do.

AaronBallman · 2021-02-10T20:23:02Z

Yeah, I'd imagine ASTDump tests would fail, so that makes a lot of sense to me. I think that generally confirms to me this is the 'right' thing to do.

Likewise.

So the question is: do we try to make that change upstream, wait for it to trickle down, and use it here? Or do we make the change here and try to upstream later (will that cause later merge conflicts for ourselves)? Or do we make the change here and not try to upstream?

erichkeane · 2021-02-10T20:24:57Z

I'd say upstream it (as review-after-commit/NFC) and apply it here as well, the conflict should be easy enough to fix.

AaronBallman · 2021-02-10T20:28:16Z

I'd say upstream it (as review-after-commit/NFC) and apply it here as well, the conflict should be easy enough to fix.

Okay, that seems reasonable enough to me (though I'd probably get rsmith to review the change pre-commit rather than go with post-commit review just because this does seem like a surprising oversight for a very common case).

@elizabethandrews -- would you like me to do the upstream work or would you like to do it? (I'm fine either way.)

erichkeane · 2021-02-10T20:35:11Z

I'd say upstream it (as review-after-commit/NFC) and apply it here as well, the conflict should be easy enough to fix.

Okay, that seems reasonable enough to me (though I'd probably get rsmith to review the change pre-commit rather than go with post-commit review just because this does seem like a surprising oversight for a very common case).

@elizabethandrews -- would you like me to do the upstream work or would you like to do it? (I'm fine either way.)

of interest, here is the patch that added that code: llvm/llvm-project@dea9d57

It appears the intent of this is to do exactly what we want, except it missed here:

VerifyIntegerConstantExpression now stores the evaluated value in ConstantExpr.
the Constant Evaluator uses the stored value of ConstantExpr when available.

There were no comments initially about this one, so it may have just fallen by the wayside.

AaronBallman · 2021-02-10T20:41:56Z

I'd say upstream it (as review-after-commit/NFC) and apply it here as well, the conflict should be easy enough to fix.

Okay, that seems reasonable enough to me (though I'd probably get rsmith to review the change pre-commit rather than go with post-commit review just because this does seem like a surprising oversight for a very common case).
@elizabethandrews -- would you like me to do the upstream work or would you like to do it? (I'm fine either way.)

of interest, here is the patch that added that code: llvm/llvm-project@dea9d57

It appears the intent of this is to do exactly what we want, except it missed here:

VerifyIntegerConstantExpression now stores the evaluated value in ConstantExpr.

the Constant Evaluator uses the stored value of ConstantExpr when available.

There were no comments initially about this one, so it may have just fallen by the wayside.

Good catch! That makes me feel more comfortable with post-commit review, thanks @erichkeane!

elizabethandrews · 2021-02-11T11:24:35Z

I'd say upstream it (as review-after-commit/NFC) and apply it here as well, the conflict should be easy enough to fix.

Okay, that seems reasonable enough to me (though I'd probably get rsmith to review the change pre-commit rather than go with post-commit review just because this does seem like a surprising oversight for a very common case).

@elizabethandrews -- would you like me to do the upstream work or would you like to do it? (I'm fine either way.)

We definitely should upstream the change since this is not a SYCL only change. However, I haven't contributed to llvm/clang enough to feel comfortable doing a post-commit review. If that's the route we're taking, I would appreciate if you could submit the change. Otherwise I can submit a patch which undergoes the normal review process upstream.

I also personally feel we should submit this PR as is (after reverting CodeGen changes and implementing review comments) and make the CodeGen changes after the upstream patch is pulled to intel/llvm. Otherwise this PR is going to have a bunch of unrelated test changes (unrelated to Intel FPGA, which is what this PR is about). I can file a tracker to make sure we don't forget to do this. Is that alright with you @erichkeane @AaronBallman ?

Signed-off-by: Elizabeth Andrews <[email protected]>

AaronBallman · 2021-02-11T12:27:16Z

I'd say upstream it (as review-after-commit/NFC) and apply it here as well, the conflict should be easy enough to fix.

Okay, that seems reasonable enough to me (though I'd probably get rsmith to review the change pre-commit rather than go with post-commit review just because this does seem like a surprising oversight for a very common case).
@elizabethandrews -- would you like me to do the upstream work or would you like to do it? (I'm fine either way.)

We definitely should upstream the change since this is not a SYCL only change. However, I haven't contributed to llvm/clang enough to feel comfortable doing a post-commit review. If that's the route we're taking, I would appreciate if you could submit the change. Otherwise I can submit a patch which undergoes the normal review process upstream.

Okay, I'm happy to work on it.

I also personally feel we should submit this PR as is (after reverting CodeGen changes and implementing review comments) and make the CodeGen changes after the upstream patch is pulled to intel/llvm. Otherwise this PR is going to have a bunch of unrelated test changes (unrelated to Intel FPGA, which is what this PR is about). I can file a tracker to make sure we don't forget to do this. Is that alright with you @erichkeane @AaronBallman ?

I was thinking that if we wanted to do it both up and downstream, we'd apply the const expr changes separately (and before) these attribute changes. So they'd be logically separate commits. However, I'm fine any which way we want to go about it.

elizabethandrews · 2021-02-11T12:33:39Z

I'd say upstream it (as review-after-commit/NFC) and apply it here as well, the conflict should be easy enough to fix.

Okay, that seems reasonable enough to me (though I'd probably get rsmith to review the change pre-commit rather than go with post-commit review just because this does seem like a surprising oversight for a very common case).
@elizabethandrews -- would you like me to do the upstream work or would you like to do it? (I'm fine either way.)

We definitely should upstream the change since this is not a SYCL only change. However, I haven't contributed to llvm/clang enough to feel comfortable doing a post-commit review. If that's the route we're taking, I would appreciate if you could submit the change. Otherwise I can submit a patch which undergoes the normal review process upstream.

Okay, I'm happy to work on it.

I also personally feel we should submit this PR as is (after reverting CodeGen changes and implementing review comments) and make the CodeGen changes after the upstream patch is pulled to intel/llvm. Otherwise this PR is going to have a bunch of unrelated test changes (unrelated to Intel FPGA, which is what this PR is about). I can file a tracker to make sure we don't forget to do this. Is that alright with you @erichkeane @AaronBallman ?

I was thinking that if we wanted to do it both up and downstream, we'd apply the const expr changes separately (and before) these attribute changes. So they'd be logically separate commits. However, I'm fine any which way we want to go about it.

Ok. That works for me. Thanks!

Signed-off-by: Elizabeth Andrews <[email protected]>

AaronBallman

LGTM!

smanna12

Thanks @elizabethandrews for the refactoring work with work_group_size attributes and saving ConstantExpr for all function attributes.

premanandrao

This isn't an area that I am familiar with, and I trust @AaronBallman's and @erichkeane's reviews of the code. I am approving this.

…3169) Currently , the behavior is inconsistent for FPGA attributes. We evaluate and store the ConstantExpr for some SYCL/FPGA attributes , and discard it for others. This patch is part 1 of making the behavior consistent. This patch saves the ConstantExpr for the following attributes. The ConstantExpr is then extracted from the Sematic attributes and used in CodeGen instead of reevaluating it. SYCLIntelNoGlobalWorkOffsetAttr, SYCLIntelMaxGlobalWorkDimAttr, SYCLIntelNumSimdWorkItemsAttr , IntelReqdSubGroupSizeAttr , SYCLIntelSchedulerTargetFmaxMhzAttr , ReqdWorkGroupSizeAttr, IntelReqdSubGroupSizeAttr, SYCLIntelMaxWorkGroupSizeAttr A consequence of this change is a change in diagnostic when the attribute argument is not an integer constant expression. The new diagnostic is an existing llvm-project diagnostic. A follow-up patch(s) will address the remaining attributes. Signed-off-by: Elizabeth Andrews [email protected]

elizabethandrews requested a review from premanandrao as a code owner February 5, 2021 18:20

elizabethandrews requested a review from AaronBallman February 5, 2021 18:20

AaronBallman reviewed Feb 5, 2021

View reviewed changes

elizabethandrews added 3 commits February 7, 2021 06:18

Fix failing lit test

3eedd5a

Signed-off-by: Elizabeth Andrews <[email protected]>

[NFC] Refactor to remove unnecessary code

ba40bd7

Signed-off-by: Elizabeth Andrews <[email protected]>

[SYCL] Save ConstantExpr for WorkGroupSize attributes

3f0e2b0

[NFC] Use saved ConstantExpr from Semantic attribute

2f2068f

Signed-off-by: Elizabeth Andrews <[email protected]>

AaronBallman reviewed Feb 9, 2021

View reviewed changes

elizabethandrews added 2 commits February 11, 2021 03:59

Implement review comments and revert CodeGen changes

a1f8d3c

Signed-off-by: Elizabeth Andrews <[email protected]>

ClangFormat changes

786ab60

Signed-off-by: Elizabeth Andrews <[email protected]>

AaronBallman mentioned this pull request Feb 11, 2021

[SYCL] Store the constant expression value into the ConstantExpr object. #3202

Closed

elizabethandrews added 2 commits February 11, 2021 10:03

Merge remote-tracking branch 'intel_llvm/sycl' into constantexpr

0af4251

Extract constant value from semantic attribute

31a1074

Signed-off-by: Elizabeth Andrews <[email protected]>

AaronBallman approved these changes Feb 11, 2021

View reviewed changes

smanna12 approved these changes Feb 11, 2021

View reviewed changes

premanandrao approved these changes Feb 11, 2021

View reviewed changes

bader merged commit c5566b2 into intel:sycl Feb 12, 2021

smanna12 mentioned this pull request Feb 16, 2021

[SYCL] [FPGA] Add mutual diagnostic for num_simd_work_items attribute in conjunction with the reqd_work_group_size attribute #3170

Merged

[SYCL] Store expression as ConstantExpr in Semantic Attribute #3169

[SYCL] Store expression as ConstantExpr in Semantic Attribute #3169

Uh oh!

Conversation

elizabethandrews commented Feb 5, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

AaronBallman left a comment

Choose a reason for hiding this comment

Uh oh!

smanna12 commented Feb 6, 2021

Uh oh!

elizabethandrews commented Feb 6, 2021

Uh oh!

elizabethandrews commented Feb 8, 2021

Uh oh!

elizabethandrews commented Feb 9, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

AaronBallman Feb 9, 2021

Choose a reason for hiding this comment

Uh oh!

elizabethandrews Feb 11, 2021

Choose a reason for hiding this comment

Uh oh!

AaronBallman Feb 9, 2021

Choose a reason for hiding this comment

Uh oh!

elizabethandrews Feb 9, 2021

Choose a reason for hiding this comment

Uh oh!

elizabethandrews Feb 9, 2021

Choose a reason for hiding this comment

Uh oh!

elizabethandrews Feb 11, 2021

Choose a reason for hiding this comment

Uh oh!

AaronBallman commented Feb 9, 2021

Uh oh!

erichkeane commented Feb 9, 2021

Uh oh!

AaronBallman commented Feb 9, 2021

Uh oh!

erichkeane commented Feb 9, 2021

Uh oh!

elizabethandrews commented Feb 9, 2021

Uh oh!

keryell commented Feb 10, 2021

Uh oh!

elizabethandrews commented Feb 10, 2021

Uh oh!

erichkeane commented Feb 10, 2021

Uh oh!

AaronBallman commented Feb 10, 2021

Uh oh!

AaronBallman commented Feb 10, 2021

Uh oh!

erichkeane commented Feb 10, 2021

Uh oh!

AaronBallman commented Feb 10, 2021

Uh oh!

erichkeane commented Feb 10, 2021

Uh oh!

AaronBallman commented Feb 10, 2021

Uh oh!

erichkeane commented Feb 10, 2021

Uh oh!

AaronBallman commented Feb 10, 2021

Uh oh!

elizabethandrews commented Feb 11, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

AaronBallman commented Feb 11, 2021

Uh oh!

elizabethandrews commented Feb 11, 2021

Uh oh!

AaronBallman left a comment

Choose a reason for hiding this comment

Uh oh!

smanna12 left a comment

elizabethandrews commented Feb 5, 2021 •

edited

Loading

elizabethandrews commented Feb 9, 2021 •

edited

Loading

elizabethandrews commented Feb 11, 2021 •

edited

Loading