[Openmp-commits] Some smaller patches.

[Hal Finkel]

----- Original Message -----
> From: "Jonathan L Peyton" <jonathan.l.peyton at intel.com>
> To: "Hal Finkel" <hfinkel at anl.gov>
> Cc: openmp-dev at dcs-maillist2.engr.illinois.edu, openmp-commits at dcs-maillist2.engr.illinois.edu
> Sent: Friday, February 13, 2015 6:02:53 PM
> Subject: Some smaller patches.
> 
> Hal,
> 
> I have some small patches here. I put the bigger ones on Phabricator.
> If you want all of these on Phabricator I’ll start doing that
> instead.
> 
> 
> 
> 1) security_flags.patch – added some flags for security on Linux and
> Mac link stages.

Are these now going to be always on by default? Can we add a build flag to disable them?

P.S. Even though this patch is small, it is also a good candidate for Phabricator because it is hard, just from the patch itself, what build targets are being modified.

> 
> 2) stack_offset.patch – changes default stack offset for threads on
> non-Mac architectures to a CACHE_LINE. This puts threads at
> different offsets from a page during creation.

For one thing, this comment explaining what is going on should really be in the code too. Second, I don't understand why this is desirable, can you please explain.

Also, some of the uses of this variable seem questionable (KMP_DEFAULT_STKOFFSET is used to set __kmp_stkoffset), and we have this in z_Linux_util.c:

#if KMP_OS_LINUX || KMP_OS_FREEBSD
    if ( __kmp_stkoffset > 0 && gtid > 0 ) {
        padding = alloca( gtid * __kmp_stkoffset );
    }
#endif

this padding variable is dead, and I'd hope that the compiler removes it.

And then, as you imply, this is used to adjust the thread's stack size:

            /* Set stack size for this thread now. */
            stack_size += gtid * __kmp_stkoffset;
...
                status = pthread_attr_setstacksize( & thread_attr, stack_size );

any while I understand that this might also affect the starting offset of subsequent thread's stacks, I don't see what would be true (I assume that the OS will create each thread's stack so that the start of the stack is really page aligned). Again, why you're multiplying by gtid is not explained. Are you trying to reduce false sharing?

In short, this needs more comments (at least).

> 
> 3) omp_flush_fix.patch – removes unused varargs from #pragma omp
> flush api function.
> 

LGTM.

Thanks again,
Hal

> 
> 
> -- Johnny
> 
> 

-- 
Hal Finkel
Assistant Computational Scientist
Leadership Computing Facility
Argonne National Laboratory