Skip to content
This repository was archived by the owner on Oct 31, 2025. It is now read-only.

Always inline calls with "not obviously legal" pointer args (instead of the opposite).#1023

Merged
eddyb merged 4 commits intoEmbarkStudios:mainfrom
LykenSol:inliner-positive-memobj
Apr 4, 2023
Merged

Always inline calls with "not obviously legal" pointer args (instead of the opposite).#1023
eddyb merged 4 commits intoEmbarkStudios:mainfrom
LykenSol:inliner-positive-memobj

Conversation

@eddyb
Copy link
Contributor

@eddyb eddyb commented Apr 3, 2023

Before this PR, the behavior of the inliner wrt pointer types in fn signatures (and pointer args) was to special-case some illegal cases and force inlining. This PR flips some of that on its head, treating everything it's not sure about as illegal, forcing inlining in all the cases not explicitly deemed legal.

Specifically for fixing #1021, the necessary change is obeying the SPIR-V spec about pointer call arguments having to be "memory object declarations" (i.e. an OpVariable, or one of the caller's own OpFunctionParameters, which transitively must be a whole OpVariable etc.).

With this PR, any pointer argument other than an OpVariable (either global or in the caller) or an OpFunctionParameter, will force inlining (even if some of those edge cases may actually be legal, we'll still be correctly conservative).

(The several commits should be reviewed separately, as the first 3 only refactor linker::inline's existing logic)

@eddyb eddyb requested a review from oisyn as a code owner April 3, 2023 15:32
@eddyb eddyb enabled auto-merge (rebase) April 3, 2023 15:33
@ProfLander
Copy link

This has been tested with the MRP + (SPIR-T HTML preview) that instigated #1021, and compiles it correctly.

Anecdotally, it's also been tested with the practical case that consumes the cons borrowing trait which highlighted the bug, is functioning as expected there as well 👍

@eddyb eddyb merged commit 1370631 into EmbarkStudios:main Apr 4, 2023
@eddyb eddyb deleted the inliner-positive-memobj branch April 4, 2023 09:16
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Inliner heuristic for "illegal pointer argument" is incomplete.

3 participants