Skip to content

Conversation

@sheino
Copy link
Contributor

@sheino sheino commented Aug 1, 2016

Added alignment pragma, which replaces ivdep pragma if running with OpenMP

@sheino
Copy link
Contributor Author

sheino commented Aug 1, 2016

Probably set_aligned_pragma needs to go somewhere else...
I just put it inside compiler as i found self.pragma_aligned var there and wanted to be consistent with having all pragma declarations inside compiler.

Thoughs @mlange05 ?

@navjotk
Copy link
Member

navjotk commented Aug 2, 2016

I think this belongs in Propagator. Also, I think the method could be made more generic. I would hold on to accepting this PR until the test comparing the save=True and save=False case as pointed out in #62 is ready and passes here.

@pvelesko
Copy link

pvelesko commented Aug 2, 2016

but all the pragmas are set up and inside compiler.py and this is a pragma as well.

@navjotk
Copy link
Member

navjotk commented Aug 2, 2016

It needs to be aware of the concepts of factors, loop counters and time steppers. All implementation choices in Propagator which might be changed very soon. compiler.py deals with the compiler interface, something we expect to not change frequently.

Why introduce this unnecessary coupling between the two?

Why does someone working on the compiler interface need to know what time steppers are?

If I change how my factors are put into the stencil, why should the compiler interface need to be changed?

If I change my loop variables, why should my compiler interface need to know about it?

@mlange05
Copy link

mlange05 commented Aug 2, 2016

Ok, so the compiler is supposed to provide the "wording" of pragmas (due to toolchain-dependent variants of ivdep originally). So at the most it should provide the string omp simd aligned, but since the whole thing needs to be parameterised, how about providing it as a function rather than a property? This way the propagator does all the sanity checking and only inserts the pragma_str, something like compiler.aligned_pragma(pragma_str).

@sheino sheino force-pushed the Alignment_pragma branch 2 times, most recently from d008114 to a28ae50 Compare August 2, 2016 15:53
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

is this equivalent to

for item in flatten(stencil.free_symbols for stencil in stencils):
    if str(item) not in factorized and item not in loop_counters + time_steppers:
        array_names.add(item)

@sheino sheino force-pushed the Alignment_pragma branch 4 times, most recently from 6c53647 to 2b08fb3 Compare August 4, 2016 12:25
@sheino
Copy link
Contributor Author

sheino commented Aug 4, 2016

@navjotk @mlange05 waiting for comments or a go if it looks alright

@sheino sheino force-pushed the Alignment_pragma branch from 2b08fb3 to fac5075 Compare August 4, 2016 14:04
@FabioLuporini
Copy link
Contributor

I pushed a small change to make everything more pythonic (also fixes the docstring)

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Was the pragma string not supposed to come from the Compiler?

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should the alignment size (:64) really be hard-coded, or should this come from the respective Compiler ?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good catch. I think we talked about this before. Such constants should be determined programmatically using sysctlbyname: http://www.manpagez.com/man/3/sysctlbyname/
In the case of cache line size you are looking for hw.cachelinesize

However, and I believe @FabioLuporini raised this previously as well, we might instead want to align to page size in which case we should use hw.pagesize so we can reduce TBL misses. The amount of wasted memory is still small.

@sheino sheino force-pushed the Alignment_pragma branch 3 times, most recently from bad76a3 to 576e117 Compare August 11, 2016 11:19
@sheino sheino closed this Aug 11, 2016
@sheino sheino reopened this Aug 11, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The use of %s without replacement value looks wrong. Can we move the (%s:%s) part of the string to the caller?

@mlange05
Copy link

Looks good now. Merging...

@mlange05 mlange05 merged commit af0eae2 into master Aug 20, 2016
@mlange05 mlange05 deleted the Alignment_pragma branch August 24, 2016 08:26
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

Successfully merging this pull request may close these issues.

7 participants