Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Hotfix/tifr #260

Merged
merged 7 commits into from
May 28, 2015
Merged

Hotfix/tifr #260

merged 7 commits into from
May 28, 2015

Conversation

maddyscientist
Copy link
Member

These are fixes required to get the TIFR MILC clover rhmc benchmark working properly, as well as minor interface fixes.

  • Renamed inconsistently named interface functions in quda.h
  • Added new milc interface function qudaResidentExtendedGaugeField for creating an returning an extended gauge field from the persistent gauge field
  • Disable reliable updates for pure double precision multi-shift solver in the milc interface (this matches the behaviour of the milc staggered interface)
  • Add doxygen comments to some missing functions in quda.h
  • Added support for backing up gauge::FloatNOrder objects for use in auto tuning
  • Clover derivative computation now uses gauge::FloatNOrder structs and now supports reconstruction
  • Added autotuning support to clover derivative and sigma trace kernels.

I think the risk in adding this functionality is minimal. Although some of the quda interfaces have changed, these are only called by the milc interface (which is internal to QUDA) and by no other external application. Thus I think this is fine to include in the 0.7.1 release.

One comment though: the additions made to gauge_field_order.h are incompatible with the changes that @nmrcardoso's gauge-fixing branch has made. It's a trivial fix though, and I'll push this fix to the feature/gauge-fix branch once this pull request has been done.

…nd restoring state to enable autotuning for kernels that destroy input data.
…ng an extended gauge field from the persistent gauge field. Fixed inconsistent naming of sime QUDA interface functions and added some missing doxygen markup for these.
…eliable updates if doing a pure double precision solver.
@mathiaswagner
Copy link
Member

I have not looked at the changes yet but about getting this in 0.7.1:
Apart from the slight risk (which is fine with me if my next if applies) I would only go for this if we do have a strong motivation. How urgent is this?

What tests has this passed?

Anyway, I will have a look at the changes.

@mathiaswagner
Copy link
Member

First thing on my mind when changing the milc interface: While it completely misses doxygen comments right now this is not what we want for the future. Please add some comments so that at least the new stuff has them,

Should things like the turned off reliable updates for pure double precision solves also be mentioned in the doxygen comments then? I think doxygen comments are use documentation and this is something that might be of interest.

void computeCloverTraceQuda(void* out, void* clover, int mu, int nu, int dim[4]);

/**
* Compute the derivative of the clover term
Copy link
Member

Choose a reason for hiding this comment

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

missing @param, might also use '@sa' ?

@maddyscientist
Copy link
Member Author

To answer the question on urgency, this is something that really should be in 0.7.1. These changes are required for the TIFR clover rhmc code to work correctly, this is an important application for a new Cray XC30 installed in Mumbai. I want them to be able to use a golden version of QUDA without having to rely on some development branch.

@@ -878,14 +878,25 @@ void qudaInvert(int external_precision,

#ifdef GPU_CLOVER_DIRAC

void* qudaCreateExtendedGaugeField(void* gauge, int geometry, int precision)
static inline void* createExtendedGaugeField(void* gauge, int geometry, int precision, int resident)
{
qudamilc_called<true>(__func__);
Copy link
Member

Choose a reason for hiding this comment

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

Just wondering whether we should us qudamilc_called<> only in exposed functions.
However, we can keep it this way for now and I will create a separate cleanup issue and we can decide whether we also want to have the qudamilc_called<false> call before exiting for all exposed functions.

But as this is only verbose output it does not prevent the merge. It just made me aware of another inconsistency in the milc interface.

Copy link
Member

Choose a reason for hiding this comment

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

Mathias, are we having the developers call? Did I miss something? Did I understand wrong the time???

El 28/5/2015, a las 21:09, Mathias Wagner [email protected] escribió:

In lib/milc_interface.cpp #260 (comment):

@@ -878,14 +878,25 @@ void qudaInvert(int external_precision,

#ifdef GPU_CLOVER_DIRAC

-void* qudaCreateExtendedGaugeField(void* gauge, int geometry, int precision)
+static inline void* createExtendedGaugeField(void* gauge, int geometry, int precision, int resident)
{
qudamilc_called(func);
Just wondering whether we should us qudamilc_called<> only in exposed functions.
However, we can keep it this way for now and I will create a separate cleanup issue and we can decide whether we also want to have the qudamilc_called call before exiting for all exposed functions.

But as this is only verbose output it does not prevent the merge. It just made me aware of another inconsistency in the milc interface.


Reply to this email directly or view it on GitHub https://github.com/lattice/quda/pull/260/files#r31267213.

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.

3 participants