Skip to content

Fix regressions in ZSTD module#3931

Merged
copybara-service[bot] merged 4 commits intogoogle:mainfrom
antmicro:zstd_job_fix
Mar 25, 2026
Merged

Fix regressions in ZSTD module#3931
copybara-service[bot] merged 4 commits intogoogle:mainfrom
antmicro:zstd_job_fix

Conversation

@magancarz
Copy link
Copy Markdown
Contributor

@magancarz magancarz commented Mar 10, 2026

This PR:

  • reverts enabling proc-scoped channels in ZSTD module, which caused regressions in builds and decoding test cases (77dcc8)
  • fixes place and route target failures by specifying larger die dimensions
  • adds a fix for opt_ir_benchmark targets by checking if a node is tracked before getting info in lazy_query_engine.h. (Fixes xls_benchmark_ir targets failing due to internal checks #3637)

Comment thread xls/modules/zstd/memory/BUILD
Comment thread xls/passes/lazy_query_engine.h Outdated
@proppy
Copy link
Copy Markdown
Member

proppy commented Mar 11, 2026

Looks like the zstd CI was cancelled after 5 hours, is that expected?

@proppy
Copy link
Copy Markdown
Member

proppy commented Mar 11, 2026

List of commits which introduced regressions to ZSTD module:

it doesn't looks 0b3b7d is being reverted to me, am I missing something?

@magancarz
Copy link
Copy Markdown
Contributor Author

magancarz commented Mar 11, 2026

Looks like the zstd CI was cancelled after 5 hours, is that expected?

No, the timeout error was caused by long-running P&R targets and I think those should be tagged as long and skipped in CI to prevent these kind of problems, that is:

  • sequence_executor_place_and_route
  • command_constructor_place_and_route
  • fse_dec_place_and_route
  • ram_mux_place_and_route
  • huffman_decoder_place_and_route
  • mem_writer_place_and_route

I'll update the pull request with relevant changes.

it doesn't looks 0b3b7d is being reverted to me, am I missing something?

Sorry, that was unclear. This commit created the Test ZSTD module job run which showed the error caused by the opt_ir_benchmark targets. Previous Test ZSTD module run on main, as seen on the Actions page, was dispatched by cc8501 and ended without errors. The error was introduced somewhere in between, probably by b20719 and 8bacd2 fixes it.

@magancarz
Copy link
Copy Markdown
Contributor Author

I've added a long tag to long-running benchmark synth / P&R targets and excluded them in the Test ZSTD module job.

Comment thread .github/workflows/modules-zstd.yml
Comment thread xls/passes/lazy_query_engine.h Outdated
@magancarz
Copy link
Copy Markdown
Contributor Author

I've dropped the commit checking if a node is tracked before getting the info since the actual fix is in #3975.

@magancarz
Copy link
Copy Markdown
Contributor Author

I've rebased changes onto the latest main, so fix from #3975 is included.

@copybara-service copybara-service Bot merged commit d0b6a43 into google:main Mar 25, 2026
3 checks passed
@kbieganski kbieganski deleted the zstd_job_fix branch March 25, 2026 16:51
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

xls_benchmark_ir targets failing due to internal checks

5 participants