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

[runtime] Fix off-by-one error in minimum integers #916

Merged
merged 1 commit into from Sep 14, 2020

Conversation

erjin
Copy link
Contributor

@erjin erjin commented Aug 25, 2020

runtime/flang/const.c defines min_int{1,2,4,8} to be -max_int{1,2,4,8}, which is wrong for the two's complement representation. As a result, the MAXLOC runtime function will compare each element in the input array with an incorrect minimum value. If the array contains -huge(integer)-1, MAXLOC will return an unexpected result.

@gklimowicz
Copy link
Contributor

From a practical perspective (how many ones-complement machines are there?), this seems OK.

It's not clear to me without more digging, though, if this is legal Fortran. The definition of model integers in the standard (F2018 subclause 16.4) defines model integers only through the equivalent of -HUGE(the integer kind under consideration).

@bryanpkc
Copy link
Collaborator

@gklimowicz I read that part of the language standard and I was wondering about the same thing. However, neither flang nor gfortran complains about the use of -HUGE(integer) - 1, and gfortran actually generates MAXLOC code that runs correctly for the test case in this PR.

@gklimowicz
Copy link
Contributor

@bryanpkc Yeah. It's hard to argue against that. I will stop. :-)

@gklimowicz gklimowicz self-requested a review August 31, 2020 23:46
Copy link
Contributor

@gklimowicz gklimowicz left a comment

Choose a reason for hiding this comment

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

Passes on OpenPOWER on LLVM 9

Copy link
Collaborator

@kiranchandramohan kiranchandramohan left a comment

Choose a reason for hiding this comment

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

LGTM. A trivial comment inline (can be ignored).

test/f90_correct/src/maxloc14.f90 Outdated Show resolved Hide resolved
runtime/flang/const.c defines min_int{1,2,4,8} to be -max_int{1,2,4,8}, which
is wrong for the two's complement representation. As a result, the MAXLOC
runtime function will compare each element in the input array with an incorrect
minimum value. If the array contains -huge(integer)-1, MAXLOC will return an
unexpected result.
Copy link
Collaborator

@shivaramaarao shivaramaarao left a comment

Choose a reason for hiding this comment

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

LGTM. thanks

@kiranchandramohan
Copy link
Collaborator

@bryanpkc please merge if no further changes required. You have approval from everyone i guess.

@bryanpkc bryanpkc merged commit 6aabdc0 into flang-compiler:master Sep 14, 2020
@bryanpkc
Copy link
Collaborator

Thanks @kiranchandramohan.

@bryanpkc bryanpkc deleted the fix-maxloc-int_min branch September 14, 2020 21:25
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.

None yet

5 participants