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

run another instcombine+sccp after licm or something like that #591

Closed
jordens opened this issue Oct 18, 2016 · 10 comments
Closed

run another instcombine+sccp after licm or something like that #591

jordens opened this issue Oct 18, 2016 · 10 comments
Assignees
Milestone

Comments

@jordens
Copy link
Member

jordens commented Oct 18, 2016

10:11 < rjo> whitequark: a quick question. https://github.com/m-labs/artiq/blob/phaser/artiq/examples/phaser/repository/demo.py even with kernel_invariants and
             fast-math, this generates lots of muldf3 and fixdfsi. anything obvious that prevents that from being const-folded?
10:12 < whitequark> set_phase and set_amplitude are syscalls, right?
10:12 < whitequark> actually, no, nevermind
10:12 < whitequark> do ARTIQ_DUMP_LLVM=foo.ll artiq_run demo.py
10:12 < whitequark> and give me the file
10:13 < rjo> http://hastebin.com/fuguwemuqa.pl
10:15 < rjo> dropping the round() does not help
10:15 < whitequark> hmmmm
10:16 < rjo> it's not urgent at all. just something i could not understand.
10:16 < whitequark> no, that's wrong
10:16 < whitequark> the round intrinsic is marked as safe to speculatively execute
10:16 < whitequark> I did that in April
10:17 < whitequark> oh, I see
10:17 < whitequark> >curl http://hastebin.com/raw/fuguwemuqa | opt -std-link-opts -S
10:17 < whitequark> observe the output
10:18 < whitequark> we need to run another instcombine+sccp after licm or something like that
10:19 < whitequark> file an issue, I'll fix it later. it's not hard, just needs some time to figure out why the existing pipeline isn't enough

http://hastebin.com/fuguwemuqa.ll

@whitequark
Copy link
Contributor

This actually already doesn't have the problem on the master/phaser branch. I believe the recent fixes for LLVM performance also addressed this issue.

@sbourdeauducq
Copy link
Member

Is that fixed in release-2?

@whitequark
Copy link
Contributor

No, since release-2 doesn't run the testcase...

@sbourdeauducq
Copy link
Member

Well, @jordens added this to the 2.1 milestone. Should it be moved to 3.0 then?

@jordens
Copy link
Member Author

jordens commented Nov 14, 2016

If it also happens to e.g. the DDS code in 2.0, which is likely, and if it is possible to backport, I'd like to see this fixed in 2.1.

@jordens
Copy link
Member Author

jordens commented Nov 20, 2016

@whitequark Is this more difficult than adding a few more passes?

@whitequark
Copy link
Contributor

@jordens The fix in the phaser branch was completely different, in fact it didn't involve adding passes at all (but making the analyses more precise); this requires migrating 2.x to LLVM 3.9.

I could add the passes as described above, but, in light of the way this was fixed in master, this seems sloppy, and I would really like to investigate first with some representative code (that runs on 2.x) whether this is the right solution, or whether there is some other bug.

@whitequark
Copy link
Contributor

Reopening as per IRC discussion.

@whitequark whitequark reopened this Nov 21, 2016
@whitequark
Copy link
Contributor

Closing as per discussion with @sbourdeauducq, in favor of asking dowstream to upgrade to 3.0.

@jordens
Copy link
Member Author

jordens commented Apr 12, 2017

Ok by me. Just need to verify that it does not occur in master.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

3 participants