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

Fix integer division in Python frontend #1196

Merged
merged 20 commits into from
Mar 20, 2023
Merged

Fix integer division in Python frontend #1196

merged 20 commits into from
Mar 20, 2023

Conversation

tbennun
Copy link
Collaborator

@tbennun tbennun commented Feb 18, 2023

Two serious symbolic issues were tracked to the following behavior:

  • symstr printed divisions as the C++ versions, even when the code is intended to be read as Python
  • The floor division operator was read by pystr_to_symbolic as floor(a/b)

This PR resolves this issue and adds an appropriate test.

Copy link
Contributor

@alexnick83 alexnick83 left a comment

Choose a reason for hiding this comment

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

Please check that you actually get the correct result for negative numbers. You may want to use py_floor or int_floor_ni.

dace/data.py Outdated Show resolved Hide resolved
dace/frontend/python/replacements.py Outdated Show resolved Hide resolved
dace/libraries/blas/nodes/dot.py Outdated Show resolved Hide resolved
dace/libraries/blas/nodes/dot.py Outdated Show resolved Hide resolved
dace/libraries/blas/nodes/ger.py Outdated Show resolved Hide resolved
dace/sdfg/propagation.py Outdated Show resolved Hide resolved
dace/sdfg/propagation.py Outdated Show resolved Hide resolved
dace/sdfg/propagation.py Outdated Show resolved Hide resolved
@@ -971,7 +971,8 @@ class BitwiseOpConverter(ast.NodeTransformer):
ast.BitXor: 'BitwiseXor',
ast.Invert: 'BitwiseNot',
ast.LShift: 'LeftShift',
ast.RShift: 'RightShift'
ast.RShift: 'RightShift',
ast.FloorDiv: 'int_floor',
Copy link
Contributor

Choose a reason for hiding this comment

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

This may be a major one, so please check this thoroughly. Python floor division rounds the remainder towards negative infinity. We have implemented this in the runtime, so you may want to call an appropriate method. From dace/runtime/include/dace/math.h:

// Computes integer floor, rounding the remainder towards negative infinity.
// https://stackoverflow.com/a/39304947
template <typename T, std::enable_if_t<std::is_integral<T>::value && std::is_signed<T>::value>* = nullptr>
static DACE_CONSTEXPR DACE_HDFI T int_floor_ni(const T& numerator, const T& denominator) {
    auto divresult = std::div(numerator, denominator);
    T corr = (divresult.rem != 0 && ((divresult.rem < 0) != (denominator < 0)));
    return (T)divresult.quot - corr;
}
template <typename T, std::enable_if_t<std::is_integral<T>::value && std::is_unsigned<T>::value>* = nullptr>
static DACE_CONSTEXPR DACE_HDFI T int_floor_ni(const T& numerator, const T& denominator) {
    T quotient = numerator / denominator;
    T remainder = numerator % denominator;
    T corr = (remainder != 0 && ((remainder < 0) != (denominator < 0)));
    return quotient - corr;
}

// Computes Python floor division
template<typename T, std::enable_if_t<std::is_integral<T>::value>* = nullptr>
static DACE_CONSTEXPR DACE_HDFI T py_floor(const T& numerator, const T& denominator) {
    return int_floor_ni(numerator, denominator);
}
template<typename T, std::enable_if_t<!std::is_integral<T>::value && std::is_floating_point<T>::value>* = nullptr>
static DACE_CONSTEXPR DACE_HDFI T py_floor(const T& numerator, const T& denominator) {
    return (T)std::floor(numerator / denominator);
}
template<typename T>
static DACE_CONSTEXPR DACE_HDFI std::complex<T> py_floor(const std::complex<T>& numerator, const std::complex<T>& denominator) {
    std::complex<T> quotient = numerator / denominator;
    quotient.real(std::floor(quotient.real()));
    quotient.imag(0);
    return quotient;
}

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

This only applies to memlets here. There is an easy fix: if we are dealing with memlets, we will generate truncation, otherwise, int_floor will generate a call to py_floor. Does that work?

Copy link
Contributor

Choose a reason for hiding this comment

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

I see. By memlets, do you mean subsets generated due to slicing expressions, e.g., A[i//5] = 3? This should be fine because it would only be incorrect if combined with negative indices. However, symbolic negative indices are not supported anyway, while constant negative indices will be evaluated with the CPython interpreter while parsing.

Copy link
Contributor

Choose a reason for hiding this comment

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

I would like to support symbolic negative indices in the future, so could you leave a NOTE somewhere in the code to warn about this behavior?

Copy link
Contributor

Choose a reason for hiding this comment

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

This would be useful towards handling Fortran codes as well. We are currently leaning towards normalizing to 0, but this does make the SDFG version of code harder to identify with the original - and the more array indexing changes, the harder debugging becomes.

@alexnick83 alexnick83 self-requested a review March 20, 2023 19:51
@tbennun tbennun enabled auto-merge March 20, 2023 19:52
@tbennun tbennun merged commit a16e328 into master Mar 20, 2023
@tbennun tbennun deleted the fix-divide branch March 20, 2023 20:48
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.

3 participants