Skip to content

Conversation

@artagnon
Copy link
Contributor

@artagnon artagnon commented May 23, 2025

-- 8< --
Trying out an idea: there are a massive number of test updates, but I aborted early due to what seems to be a bad test update. Update: some parts of LV need to be updated.

Copy link
Contributor

@fhahn fhahn left a comment

Choose a reason for hiding this comment

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

I am not sure if using InstSimplifyFolder is feasible, as during codegen the IR is in an incomplete state (phis may not have their incoming values set, branch destination may not be set, blocks with unreachable terminators). This is probably the reason for incorrect folds, as InstSimplify tries to derive information from incomplete IR and reaches incorrect conclusions.

Doing folds at code-gen time not done intentionally at the moment, as it means we dont correctly consider them during cost modeling and VPlan execution should be as simple as possible (not complete yet but working towards that)

@artagnon
Copy link
Contributor Author

artagnon commented May 24, 2025

I am not sure if using InstSimplifyFolder is feasible, as during codegen the IR is in an incomplete state (phis may not have their incoming values set, branch destination may not be set, blocks with unreachable terminators). This is probably the reason for incorrect folds, as InstSimplify tries to derive information from incomplete IR and reaches incorrect conclusions.

Yeah, that's exactly the issue. I was just discussing with @nikic, and we could have a SimplifyQuery mode that disables analysis of phis, but I think InstSimplifyFolder is still a bit too expensive to use all the time. It does irk me a bit that InstSimplifyFolder is such a nice thing that exists, but has such niche use-cases.

Doing folds at code-gen time not done intentionally at the moment, as it means we dont correctly consider them during cost modeling and VPlan execution should be as simple as possible (not complete yet but working towards that)

Although the cost-modeling argument makes sense, I'm not sure how we can prevent emitting things like add x, 0 and gep x, 0, which were the opportunities found by this patch? Can you elaborate?

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.

2 participants