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

Add "addSubShift" rule for AArch64. #738

Closed
wants to merge 2 commits into from

Conversation

XiaohongGong
Copy link
Contributor

No description provided.

@graalvmbot
Copy link
Collaborator

Hello Xiaohong Gong, thanks for contributing a PR to our project!

We use the Oracle Contributor Agreement to make the copyright of contributions clear. We don't have a record of you having signed this yet, based on your email address xiaohong -(dot)- gong -(at)- arm -(dot)- com. You can sign it at that link.

If you think you've already signed it, please comment below and we'll check.

@XiaohongGong
Copy link
Contributor Author

Hi, I'm an employee from Arm and I have signed the OCA. Can you please check and add my email to the oca list? Thank you!

@graalvmbot
Copy link
Collaborator

Xiaohong Gong has signed the Oracle Contributor Agreement (based on email address xiaohong -(dot)- gong -(at)- arm -(dot)- com) so can contribute to this repository.

@XiaohongGong
Copy link
Contributor Author

XiaohongGong commented Oct 16, 2018

Hi all,
I have added a new match rule for AArch64, which will combine the add/sub and shift instructions to one add(shift)/sub(shift) instruction. Could anyone please take a review to my patch? Thank you!

@sanzinger sanzinger self-requested a review October 18, 2018 08:19
@XiaohongGong
Copy link
Contributor Author

@sanzinger could you take a look at this patch? Thank you!

Copy link
Contributor

@sanzinger sanzinger left a comment

Choose a reason for hiding this comment

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

Thank you for your contribution. Overall this PR looks good. See my remarks inline.

@XiaohongGong
Copy link
Contributor Author

@sanzinger I have resolved the comments except for the copyright, and the new patch is updated. Can you please take a look at it? Thank you!

@XiaohongGong
Copy link
Contributor Author

@sanzinger Is there any conclusion about the correct copyright? I'm a little worried because there will be many optimizing PRs afterwards which all depends on this patch. This patch contains the basic match rule tests for AArch64. Waiting for your answer. Thank you!

@dougxc
Copy link
Member

dougxc commented Oct 30, 2018

Hi @XiaohongGong , sorry for the delay. We're trying to reach a conclusion as quickly as possible.

@XiaohongGong
Copy link
Contributor Author

@dougxc Thank you so much!

@sanzinger
Copy link
Contributor

We can keep the copyright header. I'm going to integrate this change.

@XiaohongGong
Copy link
Contributor Author

@sanzinger thank you so much!

@XiaohongGong
Copy link
Contributor Author

Hi @sanzinger, thanks for your reviewing. But is there any problems exist when integrating this change? I mean whether the merging job is conflict or any other else problems exist that need my help. Thank you!

@graalvmbot
Copy link
Collaborator

Hello XiaohongGong, thanks for contributing a PR to our project!

We use the Oracle Contributor Agreement to make the copyright of contributions clear. We don't have a record of you having signed this yet, based on your email address 44149651+xiaohonggong -(at)- users -(dot)- noreply -(dot)- github -(dot)- com. You can sign it at that link.

If you think you've already signed it, please comment below and we'll check.

@XiaohongGong
Copy link
Contributor Author

Hi @sanzinger , I have checked the latest codes in the master branch, but I have found that it is not the latest for this patch. The patch merged is the one I first committed. The newest version includes some fixed codes according to your comment. And I think they are all valuable. Is there any method to revert this merging and merge the right version? If not, I will commit a new patch which will include all these changes. Thank you!

@XiaohongGong
Copy link
Contributor Author

Hi @sanzinger , I have committed another path #781 which is a part of this patch. Could you please take a look at it? Thank you!

@sanzinger
Copy link
Contributor

Thank you for your contributions. It seems that merged PR #781 contains all changes from this PR. Hence closing.

@sanzinger sanzinger closed this Nov 26, 2018
@XiaohongGong XiaohongGong deleted the local branch November 27, 2018 01:31
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.

None yet

4 participants