Skip to content

Move one simplification call in lowering. - #9355

Open
mcourteaux wants to merge 2 commits into
mainfrom
mcourteaux/move-simplify
Open

Move one simplification call in lowering.#9355
mcourteaux wants to merge 2 commits into
mainfrom
mcourteaux/move-simplify

Conversation

@mcourteaux

Copy link
Copy Markdown
Contributor

When debugging some stuff not part of this PR, I spotted a missing simplification, which Andrew determined to be AFTER hoist_loop_invariant_xxx. This moves the simplification in the right spot.

Breaking changes

I hope none, let's see.

These do not necessarily disqualify a PR from being merged, but they should at
least be tagged with the release_notes label.

Checklist

  • Tests added or updated (not required for docs, CI config, or typo fixes)
  • Documentation updated (if public API changed)
  • Python bindings updated (if public API changed)
  • Benchmarks are included here if the change is intended to affect performance.
  • Commits include AI attribution where applicable (see Code of Conduct)

Comment thread src/Lower.cpp Outdated

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

This should be moved up above the now-final simplification

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Andrew argued that the string should change, instead of moving this. @abadams

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Suggest moving it after the custom passes and changing string to "Conceptual stmt:", because it's what you get if you compile to conceptual_stmt.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Named it:

     debug(1) << "Lowering after reaching conceptual Stmt:\n"
              << s << "\n\n";

As Claude is grepping for "Lowering after" to find the different sections of the lowering phase.

@codecov

codecov Bot commented Aug 17, 2026

Copy link
Copy Markdown

Codecov Report

❌ Patch coverage is 70.21277% with 42 lines in your changes missing coverage. Please review.
✅ Project coverage is 70.10%. Comparing base (bb5426d) to head (1040bca).
⚠️ Report is 3 commits behind head on main.

Files with missing lines Patch % Lines
src/CheckGPUCrossTalk.cpp 69.29% 28 Missing and 11 partials ⚠️
src/Lower.cpp 40.00% 1 Missing and 2 partials ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main    #9355      +/-   ##
==========================================
+ Coverage   70.08%   70.10%   +0.01%     
==========================================
  Files         259      260       +1     
  Lines       79158    79295     +137     
  Branches    19293    19328      +35     
==========================================
+ Hits        55477    55587     +110     
- Misses      17886    17927      +41     
+ Partials     5795     5781      -14     

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

@alexreinking

Copy link
Copy Markdown
Member

Deadlocks exposed here fixed by #9372

@mcourteaux
mcourteaux force-pushed the mcourteaux/move-simplify branch from 34b2370 to 84cc3b3 Compare August 21, 2026 07:45
@mcourteaux

mcourteaux commented Aug 21, 2026

Copy link
Copy Markdown
Contributor Author

Waiting for #9372 to land to update this branch, but this seems to work now.

@abadams please take note of Claude's initiative to disable a lower pass in LLVM 21, due to a bug fixed in 22 regarding SVE on ARM. I'll open a PR separately for this LLVM pass skip and remove the skips in all the tests to see if that makes the test suite pass.

@abadams

abadams commented Aug 24, 2026

Copy link
Copy Markdown
Member

Looks like it needs to be rebased. A bunch of unrelated changes are in it now.

mcourteaux and others added 2 commits August 24, 2026 14:29
@alexreinking
alexreinking force-pushed the mcourteaux/move-simplify branch 2 times, most recently from 00a65a3 to a00b0ef Compare August 24, 2026 19:37
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.

3 participants