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

[smanna12]

This is a follow up comments on #2906 (comment)

There is a rule on FPGA document that the num_simd_work_items attribute we specify must evenly divide
the work-group size we specify for the reqd_work_group_size attribute, was missed during initial
implementation of the num_simd_work_items attribute.

OpenCL spelling for num_simd_work_items attribute supports this rule.
Current implementation of SYCL attribute “num_simd_work_items” and “reqd_work_group_size” does not support this.

This patch

1. adds support for mutual diagnostic in sema for num_simd_work_items attribute
   interacting with reqd_work_group_size attribute.
2. adds tests
3. updates documentation about this new mutual diagnostic for num_simd_work_items
   attribute with the note and examples.

Signed-off-by: Soumi Manna soumi.manna@intel.com

[AaronBallman]

I think we're likely missing related changes in SemaTemplateInstantiate.cpp -- basically, anywhere we defer a check because it may be instantiation dependent, we need to then do that checking when we actually instantiate the template.

[smanna12]

I think we're likely missing related changes in SemaTemplateInstantiate.cpp -- basically, anywhere we defer a check because it may be instantiation dependent, we need to then do that checking when we actually instantiate the template.

Thanks @AaronBallman for the review. Could you please elaborate about this? Thanks.

[AaronBallman]

I think we're likely missing related changes in SemaTemplateInstantiate.cpp -- basically, anywhere we defer a check because it may be instantiation dependent, we need to then do that checking when we actually instantiate the template.

Thanks @AaronBallman for the review. Could you please elaborate about this? Thanks.

Sure! The handleFooAttr() code in SemaDeclAttr.cpp is called when we encounter a declaration, including a template declaration. From there, we're checking "are the arguments to the attribute sensible or do we need to issue a diagnostic?". However, we can't check the arguments if they're value dependent because we don't know what value they are yet. e.g.,

template <int N>
[[attr(N)]] void func();

We're still looking at the argument as N and not a concrete value from an instantiation. When the user goes to instantiate the template, say with func<4>(), that instantiation needs to be checked to make sure that [[attr(4)]] is valid. That happens from Sema::InstantiateAttrs(), and it's that instantiation-time checking code that I think is missing from this patch.

Does that help?

[smanna12]

I think we're likely missing related changes in SemaTemplateInstantiate.cpp -- basically, anywhere we defer a check because it may be instantiation dependent, we need to then do that checking when we actually instantiate the template.

Thanks @AaronBallman for the review. Could you please elaborate about this? Thanks.

Sure! The handleFooAttr() code in SemaDeclAttr.cpp is called when we encounter a declaration, including a template declaration. From there, we're checking "are the arguments to the attribute sensible or do we need to issue a diagnostic?". However, we can't check the arguments if they're value dependent because we don't know what value they are yet. e.g.,

template <int N>
[[attr(N)]] void func();

We're still looking at the argument as N and not a concrete value from an instantiation. When the user goes to instantiate the template, say with func<4>(), that instantiation needs to be checked to make sure that [[attr(4)]] is valid. That happens from Sema::InstantiateAttrs(), and it's that instantiation-time checking code that I think is missing from this patch.

Does that help?

@AaronBallman thanks for the explanation.

I tried this test case.

int foo();
template
struct func {
[[intel::num_simd_work_items(3)]]
void operator()() const;
};

template
[[intel::reqd_work_group_size(N, N, N)]] void func::operator()() const {}

func<3.f> f;

Another similar test case:

int foo();
template
struct func {
[[intel::num_simd_work_items(3)]]
[[intel::reqd_work_group_size(N, N, N)]] void fun() {}
};

func<foo()+12>f;

When I try to instantiate the template, say with func<3.f> f, or func<-3> f or func<foo()+12> f . The instantiation checks [[intel::reqd_work_group_size(N, N, N)]] is valid or not. Here every cases i see the expected diagnostic here. That is happening from [Sema::InstantiateAttrs()] where we have

if (const auto *ReqdWorkGroupSize =
dyn_cast(TmplAttr)) {
instantiateIntelSYCTripleLFunctionAttr(
*this, TemplateArgs, *ReqdWorkGroupSize, New);
continue;
}

if (const auto *SYCLIntelNumSimdWorkItems =
dyn_cast(TmplAttr)) {
instantiateIntelSYCLFunctionAttr(
*this, TemplateArgs, SYCLIntelNumSimdWorkItems, New);
continue;
}

This is a valid example but currently fails with assertion:

template
struct func {
[[intel::reqd_work_group_size(N, N, N)]]
[[intel::num_simd_work_items(3)]]
void operator()() const;
};
func<3>f;

I think this is the issue you are trying to explain here. Please correct me if i miss something.

[AaronBallman]

I think this is the issue you are trying to explain here. Please correct me if i miss something.

That looks like the case I'd expect to see fail, yes. Another test I think might likely fail in a similar way is:

template <int N>
struct func {
[[intel::reqd_work_group_size(3, 3, 3)]]
[[intel::num_simd_work_items(N)]]
void operator()() const;
};
func<3>f;

[smanna12]

This is a rebase to apply PR#3169

[smanna12]

I think this is the issue you are trying to explain here. Please correct me if i miss something.

That looks like the case I'd expect to see fail, yes. Another test I think might likely fail in a similar way is:

template <int N>
struct func {
[[intel::reqd_work_group_size(3, 3, 3)]]
[[intel::num_simd_work_items(N)]]
void operator()() const;
};
func<3>f;

basically, anywhere we defer a check because it may be instantiation dependent, we need to then do that checking when we actually instantiate the template

This seems to be an existing issue. The same problem happens if we use max_work_group_size or SYCLIntelMaxGlobalWorkDimAttr. I am planning to cover them in a separate PR so that i can address all of the impacted attributes. Currently tests related to this PR do not cause assertion and compile without any diagnostic. I have restricted all diagnostics for template function now. @AaronBallman, is that OK for you?

[AaronBallman]

I think this is the issue you are trying to explain here. Please correct me if i miss something.

That looks like the case I'd expect to see fail, yes. Another test I think might likely fail in a similar way is:

template <int N>
struct func {
[[intel::reqd_work_group_size(3, 3, 3)]]
[[intel::num_simd_work_items(N)]]
void operator()() const;
};
func<3>f;

basically, anywhere we defer a check because it may be instantiation dependent, we need to then do that checking when we actually instantiate the template

This seems to be an existing issue. The same problem happens if we use max_work_group_size or SYCLIntelMaxGlobalWorkDimAttr. I am planning to cover them in a separate PR so that i can address all of the impacted attributes. Currently tests do not cause assertion and compile without any diagnostic. I have restricted all diagnostics for template function now. @AaronBallman, is that OK for you?

I think that's fine, but I think we should coordinate our refactoring efforts because you, @elizabethandrews, and I are all working in the same general areas and that's causing a bit of problems getting this code cleaned up (hard to clean up code other people are changing to add more reliances on). I'll type up some thoughts on this and start an email discussion so we can all try to get on the same page with a long-term goal, and maybe we can find a good division of labor where we're less likely to cause merge conflicts for one another.

[smanna12]

I think this is the issue you are trying to explain here. Please correct me if i miss something.

That looks like the case I'd expect to see fail, yes. Another test I think might likely fail in a similar way is:

template <int N>
struct func {
[[intel::reqd_work_group_size(3, 3, 3)]]
[[intel::num_simd_work_items(N)]]
void operator()() const;
};
func<3>f;

basically, anywhere we defer a check because it may be instantiation dependent, we need to then do that checking when we actually instantiate the template

This seems to be an existing issue. The same problem happens if we use max_work_group_size or SYCLIntelMaxGlobalWorkDimAttr. I am planning to cover them in a separate PR so that i can address all of the impacted attributes. Currently tests do not cause assertion and compile without any diagnostic. I have restricted all diagnostics for template function now. @AaronBallman, is that OK for you?

I think that's fine, but I think we should coordinate our refactoring efforts because you, @elizabethandrews, and I are all working in the same general areas and that's causing a bit of problems getting this code cleaned up (hard to clean up code other people are changing to add more reliances on). I'll type up some thoughts on this and start an email discussion so we can all try to get on the same page with a long-term goal, and maybe we can find a good division of labor where we're less likely to cause merge conflicts for one another.

Sure, I agree.

[AaronBallman]

My comments are all NFC, so looks good to me!

[AaronBallman]

LGTM!