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

[GSoC] llvm.invariant.start tests #6802

Merged
merged 2 commits into from Aug 3, 2017
Merged

[GSoC] llvm.invariant.start tests #6802

merged 2 commits into from Aug 3, 2017

Conversation

coodie
Copy link
Contributor

@coodie coodie commented Jul 23, 2017

This PR introduces tests for changes in #6706.

record A
{
var a : int;
}
Copy link
Member

@mppf mppf Jul 24, 2017

Choose a reason for hiding this comment

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

We have some ongoing work with initializers. Can you add a local and global test using this pattern?

record A
{
  var a : int;
  proc init(arg:int) {
    a = arg;
    super.init();
  }
}

That will cause the compiler to follow some code paths that aren't exercised in your tests so far but which we expect to become widely used in the future.

Copy link
Contributor Author

@coodie coodie Jul 28, 2017

Choose a reason for hiding this comment

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

@mppf
I'm not sure what should I test as initializers make creating new records differently:

Here is how constructors work (let me use pseudo LLVM IR for that):

%tmp = alloca %A_chpl;
%localConst = alloca %A_chpl;
call construct_A(%A_chpl* %tmp, i64 %arg);
store %A_chpl %tmp, %A_chpl* %localConst

and here is how initializers work:

%localConst = alloca %A_chpl;
call init_A(%A_chpl* %localConst, i64 %arg);

Thus there is no final store which would mark that this variable is constant from this point giving opportunity to mark it using llvm.invariant.start, my algorithm covers a lot of cases because chapel compiler uses store from temporary to final a lot while in this case it doesn't happen so it doesn't do that.

Copy link
Member

Choose a reason for hiding this comment

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

@coodie - right, so in other words:

  1. we'll need to use another approach to put llvm.invariant.start after these init calls
  2. the current approach just leaves this case alone (and in particular doesn't generate incorrect IR for it)

@mppf
Copy link
Member

mppf commented Aug 1, 2017

@coodie - could you please double-check that these tests pass the way you have written them? I'm seeing testing failures with the following ones when run with start_test in an LLVM configuration with this branch merged with master and with the llvm-invariant branch.

[Error matching program output for llvm/llvm-invariant/function_local_const]
[Error matching program output for llvm/llvm-invariant/global_const]
[Error matching program output for llvm/llvm-invariant/program_constructs]

@coodie
Copy link
Contributor Author

coodie commented Aug 3, 2017

I fixed that. These changes were related to tbaa, which generation was changed.

@mppf mppf merged commit 27c1bfe into chapel-lang:master Aug 3, 2017
@mppf mppf mentioned this pull request Aug 3, 2017
mppf added a commit that referenced this pull request Aug 3, 2017
Add a SKIPIF

This is a trivial follow-on to PR #6802.
@coodie coodie changed the title llvm.invariant.start tests [GSoC] llvm.invariant.start tests Aug 28, 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