Develop j #23
Closed
Develop j #23
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
… v files.Example 'var(1:N-1) /5*0/' -> 'var(1:N-1) /(N-1)*0/
…wrappergenerator with a subclass, additional Forthon option to handle omp settings.
|
Yes. It replaced the previous one. I started with the idea of adding a
keyword threadprivate but I went with an external list since I had to parse
the entire uedge code to get all the 400 thread private variables. I will
send a new pull request with this keyword feature instead of the list.
…On Tue, Oct 13, 2020, 13:36 David Grote ***@***.***> wrote:
Hi @jguterl <https://github.com/jguterl> - Is this replacing the previous
pull request, #21 <#21>? This does
look better.
I did have the question about the ompvarlistfile - can this be handled by
adding a variable attribute (threadprivate maybe) to the variables in the
variable description files?
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#23 (comment)>, or
unsubscribe
<https://github.com/notifications/unsubscribe-auth/AEESMZHWSO2TQZRRHSU74ZTSKS245ANCNFSM4SFFP4ZQ>
.
|
dpgrote
reviewed
Oct 13, 2020
source/Forthon_builder.py
Outdated
| @@ -106,6 +110,8 @@ | |||
| else: forthonargs.append('--no2underscores') | |||
| if not writemodules: forthonargs.append('--nowritemodules') | |||
| if timeroutines: forthonargs.append('--timeroutines') | |||
| forthonargs.append('--omppkg {}'.format(omppkg)) | |||
There was a problem hiding this comment.
Can you change this so that the omp arguments are only added to forthonargs when they are specified (i.e. not None)?
dpgrote
reviewed
Oct 13, 2020
source/compilers.py
Outdated
| @@ -129,7 +129,8 @@ def __init__(self, machine=None, debug=0, fcompname=None, fcompexec=None, implic | |||
| self.popt = '-g' | |||
| self.extra_link_args += ['-g'] | |||
| self.extra_compile_args += ['-g', '-O0'] | |||
|
|
|||
| if omp: | |||
There was a problem hiding this comment.
The command line flag "-fopenmp" is gnu specific. Can you move this into the *_gfortran routines? On intel, the flag is "-qopenmp".
|
Yes!
…On Tue, Oct 13, 2020, 14:03 David Grote ***@***.***> wrote:
***@***.**** commented on this pull request.
------------------------------
In source/compilers.py
<#23 (comment)>:
> @@ -129,7 +129,8 @@ def __init__(self, machine=None, debug=0, fcompname=None, fcompexec=None, implic
self.popt = '-g'
self.extra_link_args += ['-g']
self.extra_compile_args += ['-g', '-O0']
-
+ if omp:
The command line flag "-fopenmp" is gnu specific. Can you move this into
the *_gfortran routines? On intel, the flag is "-qopenmp".
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#23 (review)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/AEESMZE6CQA4NONL4S7SD5LSKS6DRANCNFSM4SFFP4ZQ>
.
|
|
Do you need cray too?
…On Tue, Oct 13, 2020, 14:07 Jerome Guterl ***@***.***> wrote:
Yes!
On Tue, Oct 13, 2020, 14:03 David Grote ***@***.***> wrote:
> ***@***.**** commented on this pull request.
> ------------------------------
>
> In source/compilers.py
> <#23 (comment)>:
>
> > @@ -129,7 +129,8 @@ def __init__(self, machine=None, debug=0, fcompname=None, fcompexec=None, implic
> self.popt = '-g'
> self.extra_link_args += ['-g']
> self.extra_compile_args += ['-g', '-O0']
> -
> + if omp:
>
> The command line flag "-fopenmp" is gnu specific. Can you move this into
> the *_gfortran routines? On intel, the flag is "-qopenmp".
>
> —
> You are receiving this because you were mentioned.
> Reply to this email directly, view it on GitHub
> <#23 (review)>,
> or unsubscribe
> <https://github.com/notifications/unsubscribe-auth/AEESMZE6CQA4NONL4S7SD5LSKS6DRANCNFSM4SFFP4ZQ>
> .
>
|
dpgrote
reviewed
Oct 13, 2020
| @@ -308,6 +308,9 @@ def processfile(packname, filename, othermacros=[], timeroutines=0): | |||
| elif text[i] == "'": i = re.search("'", text[2:]).start() + 2 | |||
| i = re.search('/', text[i:]).start() + i | |||
| data = text[0:i+1] | |||
| if data.count('(')==1 and data.count(')')==1: | |||
There was a problem hiding this comment.
Can you add a comment about what this is doing? I'm not sure how it works since it is doing an eval on the string between the parenthesis, but that variable is not know at this time.
|
Ok. Functionality is described in the commit message.
…On Tue, Oct 13, 2020, 14:15 David Grote ***@***.***> wrote:
***@***.**** commented on this pull request.
------------------------------
In source/interfaceparser.py
<#23 (comment)>:
> @@ -308,6 +308,9 @@ def processfile(packname, filename, othermacros=[], timeroutines=0):
elif text[i] == "'": i = re.search("'", text[2:]).start() + 2
i = re.search('/', text[i:]).start() + i
data = text[0:i+1]
+ if data.count('(')==1 and data.count(')')==1:
Can you add a comment about what this is doing? I'm not sure how it works
since it is doing an eval on the string between the parenthesis, but that
variable is not know at this time.
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#23 (review)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/AEESMZBLWIW5JLAKREDAIULSKS7NNANCNFSM4SFFP4ZQ>
.
|
|
If you know the cray openmp option then go ahead and add it. |
… .v files. Two arguments remaining: --omp and --ompdebug
dpgrote
reviewed
Oct 19, 2020
source/Forthon_options.py
Outdated
| @@ -101,6 +101,11 @@ | |||
| parser.add_option('--writemodules', action='store_true', default=True, dest='writemodules') | |||
| parser.add_option('--nowritemodules', action='store_false', default=True, dest='writemodules', help="Don't write out the module definitions. Useful if the modules have been written already. Note that if variables of derived type are used, the original code will need to be modified. See example2. Also note that if this option is used, no checks are made to ensure the consistency between the interface file description and the actual module.") | |||
|
|
|||
| # OMP related options | |||
There was a problem hiding this comment.
I have the options listed in alphabetical order. Can you move these to between '--macro' and '--pkgbase'?
|
Thanks for making the changes! This all looks good, though I have another request. I prefer the code to use some of the pep8 stylistic standards. This keeps the code looking unified and I believe easier to read. The standards I use are having spaces around equals signs, "x = y", a space after commas, "f(a, b)", and no spaces at the ends of lines. Can you make these changes? Then this PR can be merged. Thanks! |
|
I will resubmit a new pull request with clean coding (using autopip8) and
threadprivate attribute read from v files directly. Please discard the
previous pull request.
I also added the generation of a new routine to set to zero all
threadprivate variables (this is needed when variables are not properly
initialized in each thread but called later from a thread under the
assumption that they are equal to zero by default, which is only the case
for the master thread. This behavior is generally speaking compiler
dependent even on the master thread).
I added a condition to add the flag fopenmp to all compilers except the
intel one with qopenmp so it will throw an error if fopenmp is the wrong
flag (better than being ignored IMO). That can be replaced by the correct
flag or by raising an error in Forthon.
…On Mon, Oct 19, 2020 at 9:51 AM David Grote ***@***.***> wrote:
Thanks for making the changes! This all looks good, though I have another
request. I prefer the code to use some of the pep8 stylistic standards.
This keeps the code looking unified and I believe easier to read. The
standards I use are having spaces around equals signs, "x = y", a space
after commas, "f(a, b)", and no spaces at the ends of lines. Can you make
these changes? Then this PR can be merged. Thanks!
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#23 (comment)>, or
unsubscribe
<https://github.com/notifications/unsubscribe-auth/AEESMZBOBBC53MQLEW2E56LSLRVC3ANCNFSM4SFFP4ZQ>
.
|
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This is a pull request for the addition of openmp features to Forthon. This add-on provides methods to PyWrap to create threadprivate variables and associate, copy and nullify those threadprivate variables in each thread.