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

Upstream SelectionDAGBuilder::lowerInvokable Invoke parameter #126

Open
JosephTremoulet opened this issue Dec 30, 2015 · 0 comments
Open

Comments

@JosephTremoulet
Copy link
Contributor

See e.g. this TODO.

We hit an issue with 8cec2f2 introducing an InvokeStateMap and looking up InvokeInst keys in lowerInvokable from the CallLoweringInfo parameter, which causes trouble for LLILC since the call to lowerInvokable that statepoints (and patchpoints) use doesn't attach the CallSite to the CallLoweringInfo.

The two means of creating CallLoweringInfos (i.e. with and without the CallSite) have existed for as long as CallLoweringInfo has. The EH state map needs to use InvokeInsts as keys now because there are no longer EndPads and so it can't just use the unwind destination like it did previously. The InvokeInst is needed in SelectionDAGBuilder::lowerInvokable, which is a private helper method. It has two callers, one of which is for statepoints/patchpoints and the other of which always passes a CallLoweringInfo with a CallSite. To me, it makes more sense to just add the InvokeInst as a parameter to the helper, to avoid upsetting expectations in the call lowering code about what it means when a CallLoweringInfo has a CallSite attached to it and to avoid expanding CallLoweringInfo to include EH state map info that's only relevant for funclet EH.

Since the problem only arises with the combination of statepoint invokes and funclet EH, there won't be a good way to add a lit test demonstrating the issue (which should be included when this change is upstreamed) until #105 has been implemented and upstreamed; statepoint invokes can't be lowered without it. So this issue depends on that one.

@JosephTremoulet JosephTremoulet self-assigned this Dec 30, 2015
@JosephTremoulet JosephTremoulet removed their assignment Sep 14, 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

No branches or pull requests

1 participant