wsmoses / Tapir-LLVM

Tapir extension to LLVM for optimizing Parallel Programs
Other
131 stars 24 forks source link

Inconsistent grain size pragma syntax #90

Open ehein6 opened 5 years ago

ehein6 commented 5 years ago

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 commented 5 years ago

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 commented 5 years ago

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

skuntz commented 5 years ago

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 commented 5 years ago

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 commented 5 years ago

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.