Skip to content

Conversation

@ajocksch
Copy link
Contributor

@ajocksch ajocksch commented Jun 22, 2018

This PR also provides a small correction of automatic_arrays.py.

Closes #248
Closes #249

@vkarak
Copy link
Contributor

vkarak commented Jun 22, 2018

@ajocksch Please associate this PR with the issue it solves.

@vkarak vkarak self-requested a review June 22, 2018 09:40
@vkarak vkarak changed the title added OpenACC+CUDA+C++ check; small correction of automatic_arrays.py [test] Add OpenACC/CUDA/C++ test from MCH Jun 27, 2018
@vkarak
Copy link
Contributor

vkarak commented Jun 27, 2018

@ajocksch Are the failures on Kesch expected?

@vkarak vkarak changed the title [test] Add OpenACC/CUDA/C++ test from MCH [test] New OpenACC/CUDA/C++ test from MCH Jun 27, 2018
@vkarak vkarak requested a review from teojgo June 27, 2018 13:45
Copy link
Contributor

@vkarak vkarak left a comment

Choose a reason for hiding this comment

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

Apart from my specific comments, I have the following more generic ones:

  1. The test provided by MCH supports also two test cases (MPI and no MPI) without OpenACC. Should we generate tests for these as well? The corresponding issues do not require that, though.
  2. Are the failures on Kesch expected?



@rfm.parameterized_test(['mpi'], ['nompi'])
class OpenaccCudaMpiCppstd(rfm.RegressionTest):
Copy link
Contributor

Choose a reason for hiding this comment

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

The class name should not have the Mpi inside it because you are also generating a test that is not using MPI. This is confusing.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done


@rfm.parameterized_test(['mpi'], ['nompi'])
class OpenaccCudaMpiCppstd(rfm.RegressionTest):
def __init__(self, withmpi):
Copy link
Contributor

Choose a reason for hiding this comment

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

The withmpi should be a boolean, not a string.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done

self.valid_prog_environs = ['PrgEnv-cray', 'PrgEnv-pgi']
if self.current_system.name in ['daint', 'dom']:
self.modules = ['craype-accel-nvidia60']
self._pgi_flags = '-O2 -acc -ta=tesla:cc60 -Mnorpath -lstdc++'
Copy link
Contributor

Choose a reason for hiding this comment

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

Why the -lstdc++ is needed here?

Copy link
Contributor

@teojgo teojgo Jul 5, 2018

Choose a reason for hiding this comment

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

It throws link error std_cpp_call.o:(.eh_frame+0x12): undefined reference to __gxx_personality_v0 if the options is not there.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

this for some reason required

std_cpp_call.o:(.eh_frame+0x12): undefined reference to `__gxx_personality_v0'

I could also try to move it into the makefile

Copy link
Contributor

Choose a reason for hiding this comment

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

It's fine here. No problem.

self._nvidia_sm = '37'

if withmpi == 'mpi':
self.mpiflag = ' -DUSEMPI'
Copy link
Contributor

Choose a reason for hiding this comment

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

You could also make this "private" as with the _nvidia_sm. Also the flag should be USE_MPI, not USEMPI.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thank you!

Copy link
Contributor

@teojgo teojgo left a comment

Choose a reason for hiding this comment

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

lgtm apart from the changes proposed by @vkarak

@ajocksch
Copy link
Contributor Author

ajocksch commented Jul 5, 2018

currently the check with pgi+mpi fails, the reeason should be the wrong PrgEnv modules; this should be fixed when merging into the master

@ajocksch
Copy link
Contributor Author

ajocksch commented Jul 5, 2018

about 1.: Do you mean to compile with -hnoacc? I think this is not possible since a cuda kernel is called, we would need to provide a cpu version of that part of the code as well.

@teojgo
Copy link
Contributor

teojgo commented Jul 6, 2018

@jenkins-cscs retry all

@teojgo
Copy link
Contributor

teojgo commented Jul 6, 2018

lgtm. @ajocksch are the kesch failures expected?

@ajocksch
Copy link
Contributor Author

ajocksch commented Jul 6, 2018

#348 needs to be merged in the master first

Copy link
Contributor

@vkarak vkarak left a comment

Choose a reason for hiding this comment

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

Please change the name of the class, and I will approve.



@rfm.parameterized_test([True], [False])
class OpenaccCudaMpiNoMPICppstd(rfm.RegressionTest):
Copy link
Contributor

Choose a reason for hiding this comment

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

The name is weird! Why not just OpenaccCudaCpp, which is in accordance with the issue as well?

Copy link
Contributor

Choose a reason for hiding this comment

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

@teojgo Can you change the name of this class? Other than this, this is ready to be merged.

Copy link
Contributor

Choose a reason for hiding this comment

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

I don't think the automatically generated name will be very meaningful for this one: OpenaccCudaCpp_False and OpenaccCudaCpp_True. So it should be set manually.

@codecov-io
Copy link

codecov-io commented Jul 10, 2018

Codecov Report

Merging #342 into master will decrease coverage by 0.02%.
The diff coverage is n/a.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #342      +/-   ##
==========================================
- Coverage   91.12%   91.09%   -0.03%     
==========================================
  Files          68       68              
  Lines        8244     8244              
==========================================
- Hits         7512     7510       -2     
- Misses        732      734       +2
Impacted Files Coverage Δ
reframe/core/config.py 82.72% <0%> (-1.82%) ⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 2018a71...fdd50af. Read the comment docs.

@vkarak
Copy link
Contributor

vkarak commented Jul 17, 2018

@ajocksch Can you check if the failures on Kesch are expected? IMO, perhaps they are not. If they are, though, I can merge this immediately.

@vkarak vkarak removed this from the ReFrame sprint 2018w35 milestone Sep 4, 2018
Vasileios Karakasis added 2 commits October 8, 2018 11:51
@vkarak
Copy link
Contributor

vkarak commented Oct 8, 2018

@jenkins-cscs retry dom

@vkarak vkarak added this to the Upcoming sprint milestone Oct 8, 2018
@vkarak vkarak merged commit 9069bab into master Oct 8, 2018
@vkarak vkarak deleted the checks/mch_openacc_cuda_mpi_cppstd branch October 8, 2018 15:44
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants