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

Inline more type test BIFs in HiPE #1718

Merged
merged 2 commits into from
Mar 1, 2018

Conversation

michalmuskala
Copy link
Contributor

I tried running HiPE tests and they all pass, but I'm not really sure they actually hit the code and I'm not sure how to run regular compiler tests, but leveraging HiPE.

@rickard-green rickard-green added the team:VM Assigned to OTP team VM label Feb 19, 2018
@sverker sverker self-assigned this Feb 19, 2018
@sverker
Copy link
Contributor

sverker commented Feb 20, 2018

Could you say something about the rationale. To me it looks like either this PR is missing something or you're enabling something that was already there.

@margnus1 @kostis Do the HiPE crowd you have any thoughts?

@sverker
Copy link
Contributor

sverker commented Feb 20, 2018

A good general test for hipe is to build with ./otp_build setup --enable-native-libs and then run emulator, stdlib and kernel tests for example.

If the change affects code loading then just running kernel/test/code_SUITE:upgrade will provoke a lot of load/upgrade/delete/purge scenarios.

@michalmuskala
Copy link
Contributor Author

As far as I can tell, the #icode_type{} instructions can be produced from multiple sources. One way is when translating test instructions from BEAM code. That's why the instructions like a test for a map are already handled properly and there's no change required anywhere deeper in the HiPE compiler.

But the BEAM format might not translate all BIFs to test instructions and there's also the possibility of compiling from Core Erlang. In those cases, the pass I changed would translate the BIF calls to icode instructions. The change I made, makes it so that change happens in more cases.

@kostis
Copy link
Contributor

kostis commented Feb 20, 2018

I think this is an OK change. However, you need to correct the is_bitsting case to read is_bitstring.

While at it, perhaps it will be a good idea to have the cases sorted alphabetically so that they correspond to the order that these tests are mentioned in the comment (that you also changed).

@michalmuskala
Copy link
Contributor Author

michalmuskala commented Feb 23, 2018

I pushed the typo fix and also sorted alphabetically the tests both in the comment and in the implementation.

I tried running the tests with --enable-native-libs and I got some failures, but I also got them on current master, so I don't think those changes affected it.

@sverker sverker added enhancement testing currently being tested, tag is used by OTP internal CI labels Feb 23, 2018
@sverker
Copy link
Contributor

sverker commented Feb 23, 2018

I've put it in our tests.
But for future PRs, consider separate commits to not "hide" small fixes (like the typo) in larger refactorings (like the sort). It makes review and future archeology easier.

@sverker sverker merged commit aa4fcf9 into erlang:master Mar 1, 2018
@michalmuskala michalmuskala deleted the hipe-inline-bifs branch March 1, 2018 10:24
This pull request was closed.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement team:VM Assigned to OTP team VM testing currently being tested, tag is used by OTP internal CI
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants