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 codegen for comparisons to be able to handle refs #7065

Merged
merged 6 commits into from
Aug 22, 2017

Conversation

e-kayrakli
Copy link
Contributor

Without this patch codegen seems to omit dereferences for a in the following snippet:

proc foo() {
  var a = 10;
  on Locales[0] {
    writeln(a>10);
    writeln(a>=10);
    writeln(a<10);
    writeln(a<=10);
  }
}

foo();

Such code compiled fine with --no-denormalize, since the dereferencing was handled by PRIM_MOVE. However, since denormalize removes that PRIM_MOVE this code was compiled without derefrenceing causing pointer vs integer comparison.

It seems that ref intent for a is inserted post denormalize, so there doesn't seem to be a direct way of handling this at denormalize time. Moreover, such issues are handled at codegen time for arithmetic operations. I followed the same strategy for comparisons.

This PR also adds a test that generates faulty code on master.

(I can't easily reproduce this issue without on statements. I tried to do it with ref variables but it seems to work fine for those)

Tests:

  • release/examples/hello*chpl passed with GASNet
  • users/engin/refComparison.chpl passed with GASNet
  • Full GASNet
  • Full LLVM (?)

@e-kayrakli
Copy link
Contributor Author

This was encountered by @LouisJenkinsCS.

Also, I can't run paratest myself.

@benharsh benharsh self-requested a review August 21, 2017 21:12
@benharsh
Copy link
Member

benharsh commented Aug 21, 2017

I'll start some paratesting:

  • full local
  • full gasnet
  • gasnet llvm examples/

@@ -0,0 +1 @@
CHPL_COMM!=none
Copy link
Member

Choose a reason for hiding this comment

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

Does this test really need a skipif?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hmm.. not that it fails with CHPL_COMM!=none. Just that the on statement is optimized away (I think) with that environment, thus it doesn't test anything new. In other words, with CHPL_COMM=none this test passes without this PR, too.

I can remove it if you want.

Copy link
Member

Choose a reason for hiding this comment

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

Should it really be "CHPL_COMM==none" , which would only run the test in multilocale configurations?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Oh boy :( You are right, sorry about the confusion. Do you want me to fix it or remove it altogether?

Copy link
Member

Choose a reason for hiding this comment

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

Let's just remove it

Copy link
Member

@benharsh benharsh left a comment

Choose a reason for hiding this comment

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

Other than the skipif, these changes look good to me.

Testing also looks good.

@benharsh benharsh merged commit 405f600 into chapel-lang:master Aug 22, 2017
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

2 participants