Skip to content
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.

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

Inconsistent grain size pragma syntax #90

Open
ehein6 opened this issue Jul 19, 2019 · 5 comments
Open

Inconsistent grain size pragma syntax #90

ehein6 opened this issue Jul 19, 2019 · 5 comments

Comments

@ehein6
Copy link

ehein6 commented Jul 19, 2019

Tapir's expected syntax for the grain size pragma doesn't match other Cilk compilers.

Example:

#include <cilk/cilk.h>
long array[10000];
void foo() {
    #pragma cilk grainsize = 100
    cilk_for (long i = 0; i < 10000; ++i) {
        array[i] = 0;
    }
}

Error:

<source>:6:28: error: expected expression
    #pragma cilk grainsize = 100
                           ^
<source>:6:28: warning: extra tokens at end of '#pragma cilk grainsize' - ignored [-Wignored-pragmas]
1 warning and 1 error generated.
Compiler returned: 1

Removing the equals sign (i.e. #pragma cilk grainsize 100) fixes the error, but breaks compatibility with other Cilk compilers like gcc 5.5:

<source>: In function 'void foo()':
<source>:6:28: error: expected '=' before numeric constant
     #pragma cilk grainsize 100
                            ^
Compiler returned: 1

Intel's definition of Cilk required the equals sign, see https://www.cilkplus.org/sites/default/files/open_specifications/cilk_plus_language_specification_0_9.pdf.

@neboat
Copy link
Collaborator

neboat commented Jul 24, 2019

Thanks for bringing this up.

When we originally added the grainsize pragma, we opted for a simpler syntax that was more in keeping with other clang pragmas. On our end, we've generally found that issues with backwards compatibility on the grainsize pragma are straightforward to overcome. If need be, however, it shouldn't be too onerous to add support for the = in the pragma.

In general, backwards compatibility is something we're considering carefully as we develop OpenCilk and revisit decisions regarding Cilk syntax and semantics. So please let us know about any concerns you have regarding Cilk syntax and semantics.

@ehein6
Copy link
Author

ehein6 commented Sep 11, 2019

Here's a patch from Shannon that allows either syntax:
0001-Handle-sign-in-pragma-grainsize-for-backwards-compat.patch.txt

@skuntz
Copy link

skuntz commented Oct 22, 2019

Tapir also requires that the grainsize be an integral constant expression. Many of our codes compute the grainsize at run time which is no longer supported.

neboat added a commit to wsmoses/Tapir-Clang that referenced this issue Oct 22, 2019
…x. Thanks to @skuntz for the essential code for this patch.  This commit partially addresses wsmoses/Tapir-LLVM#90.
neboat added a commit to wsmoses/Tapir-Clang that referenced this issue Oct 22, 2019
…x. Thanks to @skuntz for the essential code for this patch.  This commit partially addresses wsmoses/Tapir-LLVM#90.
neboat added a commit to wsmoses/Tapir-Clang that referenced this issue Oct 22, 2019
…x. Thanks to @skuntz for the essential code for this patch.  This commit partially addresses wsmoses/Tapir-LLVM#90.
@neboat
Copy link
Collaborator

neboat commented Oct 23, 2019

Yes, Tapir is currently limited to constant (expression) grainsizes. Arbitrary grainsizes can be implemented by hand using manual loop stripmining.

I'm curious what kinds of runtime computations you're using for grainsizes. I ask because, with Cilk Plus, we would often see a poor use of runtime-computed grainsizes that was often a pessimization for the program's performance.

@ehein6
Copy link
Author

ehein6 commented Oct 23, 2019

In one case it was a performance benchmark where we wanted to control exactly how many tasks cilk_for was creating. In other codes, when it is known that each loop iteration does the same amount of work, we try to minimize the overhead of task creation by creating exactly one task per execution context. But in general I agree that runtime-computed grain sizes are not good coding practice.

neboat added a commit to OpenCilk/opencilk-project that referenced this issue Jan 15, 2020
…x. Thanks to @skuntz for the essential code for this patch.  This commit partially addresses wsmoses/Tapir-LLVM#90.
neboat added a commit to OpenCilk/opencilk-project that referenced this issue Jan 15, 2020
…x. Thanks to @skuntz for the essential code for this patch.  This commit partially addresses wsmoses/Tapir-LLVM#90.
neboat added a commit to OpenCilk/opencilk-project that referenced this issue Jan 15, 2020
…x. Thanks to @skuntz for the essential code for this patch.  This commit partially addresses wsmoses/Tapir-LLVM#90.
neboat added a commit to OpenCilk/opencilk-project that referenced this issue Jan 15, 2020
…x. Thanks to @skuntz for the essential code for this patch.  This commit partially addresses wsmoses/Tapir-LLVM#90.
stelleg pushed a commit to lanl/kitsune that referenced this issue May 27, 2020
…x. Thanks to @skuntz for the essential code for this patch.  This commit partially addresses wsmoses/Tapir-LLVM#90.
stelleg pushed a commit to stelleg/llvm-project that referenced this issue Oct 13, 2020
…x. Thanks to @skuntz for the essential code for this patch.  This commit partially addresses wsmoses/Tapir-LLVM#90.
neboat added a commit to neboat/opencilk-project that referenced this issue Jun 13, 2022
…x. Thanks to @skuntz for the essential code for this patch.  This commit partially addresses wsmoses/Tapir-LLVM#90.
tarunprabhu pushed a commit to tarunprabhu/kitsune that referenced this issue Apr 4, 2023
…x. Thanks to @skuntz for the essential code for this patch.  This commit partially addresses wsmoses/Tapir-LLVM#90.
tarunprabhu pushed a commit to tarunprabhu/kitsune that referenced this issue Jul 25, 2023
…x. Thanks to @skuntz for the essential code for this patch.  This commit partially addresses wsmoses/Tapir-LLVM#90.
tarunprabhu pushed a commit to tarunprabhu/kitsune that referenced this issue Sep 26, 2023
…x. Thanks to @skuntz for the essential code for this patch.  This commit partially addresses wsmoses/Tapir-LLVM#90.
tarunprabhu pushed a commit to tarunprabhu/kitsune that referenced this issue Oct 12, 2023
…x. Thanks to @skuntz for the essential code for this patch.  This commit partially addresses wsmoses/Tapir-LLVM#90.
neboat added a commit to neboat/opencilk-project that referenced this issue Nov 28, 2023
…x. Thanks to @skuntz for the essential code for this patch.  This commit partially addresses wsmoses/Tapir-LLVM#90.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants