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 ICE in ArithmeticIF for INT*8 type #845

Merged
merged 1 commit into from
Aug 27, 2020

Conversation

kiranchandramohan
Copy link
Collaborator

When half precision reals were introduced the table (aif) used in the
ILI codegen for ArithmeticIF was increased in size and the offset for
INT8 increased by one. But the entry for half precision was not
inserted, this causes an ICE for INT
8 type. The fix is to introduce
the entry for the half precision type before the INT*8 type.

Fix for #844

When half precision reals were introduced the table (aif) used in the
ILI codegen for ArithmeticIF was increased in size and the offset for
INT*8 increased by one. But the entry for half precision was not
inserted, this causes an ICE for INT*8 type. The fix is to introduce
the entry for the half precision type before the INT*8 type.
@gklimowicz gklimowicz self-requested a review December 13, 2019 17:41
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.

We've had the folks here look at it, and they are OK with this change. Thanks, @kiranchandramohan !

@kiranchandramohan
Copy link
Collaborator Author

Thanks @gklimowicz for having a look. Also, appreciate the quick response.

@kiranchandramohan
Copy link
Collaborator Author

@shivaramaarao FYI, I believe 859 (is it from AMD?) also addresses the same issue. But I feel this patch's approach of adding the missing entry is probably better than deleting the entry. Let me know in case you think differently.
#859

@shivaramaarao
Copy link
Collaborator

@shivaramaarao FYI, I believe 859 (is it from AMD?) also addresses the same issue. But I feel this patch's approach of adding the missing entry is probably better than deleting the entry. Let me know in case you think differently.
#859

We checked this patch and it works fine with our testing. LGTM

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

@kiranchandramohan
Copy link
Collaborator Author

@gklimowicz Since you have approved it previously is it safe to assume that you have run the tests?

@gklimowicz
Copy link
Contributor

Yes, I probably ran them in December, but let me make sure and run them again specifically on Power.

@gklimowicz gklimowicz self-requested a review August 27, 2020 18:51
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.

This still works on Power and LLVM 9. Approved.

@kiranchandramohan kiranchandramohan merged commit 55a4534 into flang-compiler:master Aug 27, 2020
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