This is an archive of the discontinued LLVM Phabricator instance.

[IPCP] Bail on extractvalue's with more than 1 index.
ClosedPublic

Authored by craig.topper on Oct 30 2019, 11:49 PM.

Details

Summary

The replacement code only looks at the first index of the
extractvalue. If there are additional indices we'll end
up doing a bad replacement.

This only happens if the function returns a nested struct. Not
sure if clang ever generates such code. The original report came
from ispc.

Fixes PR43857

Event Timeline

craig.topper created this revision.Oct 30 2019, 11:49 PM
Herald added a project: Restricted Project. · View Herald TranscriptOct 30 2019, 11:49 PM
Harbormaster completed remote builds in B40326: Diff 227229.
jdoerfert accepted this revision.Oct 31 2019, 9:06 AM

Besides the point: I didn't know anyone was still using this pass. Actually I was going to propose to delete it soon. Is there a desire to keep it or would people switch to IP-SCCP and/or the Attributor (assuming they subsume the functionality)? The one reason to keep this pass is that it is simple and "fast" but it's questionable if that is useful if we run either of the other ones anyway.

The patch LGTM.

This revision is now accepted and ready to land.Oct 31 2019, 9:06 AM
This revision was automatically updated to reflect the committed changes.

Besides the point: I didn't know anyone was still using this pass. Actually I was going to propose to delete it soon. Is there a desire to keep it or would people switch to IP-SCCP and/or the Attributor (assuming they subsume the functionality)? The one reason to keep this pass is that it is simple and "fast" but it's questionable if that is useful if we run either of the other ones anyway.

The patch LGTM.

I assume that I was added as reviewer due to being the last person who contributed to this pass.
It looks like the fuzzy test framework we use downstream (out-of-tree) sometimes add this pass to the opt pipeline. I blame that on historical reasons, and it is nothing that we really depend on.
So do not see my contribution earlier this year as a reason for keeping the pass around. I have no clue if the reporter of PR43857 is using this pass for something less exotic than fuzzy testing.

Besides the point: I didn't know anyone was still using this pass. Actually I was going to propose to delete it soon. Is there a desire to keep it or would people switch to IP-SCCP and/or the Attributor (assuming they subsume the functionality)? The one reason to keep this pass is that it is simple and "fast" but it's questionable if that is useful if we run either of the other ones anyway.

The patch LGTM.

I assume that I was added as reviewer due to being the last person who contributed to this pass.
It looks like the fuzzy test framework we use downstream (out-of-tree) sometimes add this pass to the opt pipeline. I blame that on historical reasons, and it is nothing that we really depend on.
So do not see my contribution earlier this year as a reason for keeping the pass around. I have no clue if the reporter of PR43857 is using this pass for something less exotic than fuzzy testing.

The person who filed PR43857 sits across the aisle from me. He works on ISPC (Intel SPMD Program Compiler) which uses this pass. This may be a historical artifact in their code base where they build their own pass pipeline and they didn't know the pass had been superceded by IP-SCCP. I've pass that information along to him.