Skip to content

Fix for redundant break statements in nested scopes.#4082

Draft
copybara-service[bot] wants to merge 1 commit intomainfrom
test_896698882
Draft

Fix for redundant break statements in nested scopes.#4082
copybara-service[bot] wants to merge 1 commit intomainfrom
test_896698882

Conversation

@copybara-service
Copy link
Copy Markdown

Fix for redundant break statements in nested scopes.

Generated by Gemini. Its explanation:

✦ The PipelinedLoopOuterExit test case fails when the break on line 10929 of the test file is present because of a bug in xls/contrib/xlscc/translator.cc where
relative_break_condition was being overwritten by subsequent break statements instead of being accumulated.

Root Cause Analysis
In translator.cc, within the GenerateIR_Stmt function, the handling for clang::BreakStmt and clang::ContinueStmt used simple assignment (=) for relative_break_condition and
relative_continue_condition.

1 // Original buggy code in translator.cc
2 context().relative_break_condition = context().relative_condition_bval(loc);

When a loop body contains multiple break statements, such as:

1 if (x > 10) {
2 if (x == 55) {
3 exit_function = true;
4 break; // Break 1
5 }
6 break; // Break 2
7 }
The translator processes both break statements. When x == 55, Break 1 correctly sets the relative_break_condition. However, the translator continues to process Break 2. Although Break 2 is
logically unreachable in C++ when Break 1 is taken, the translator still evaluates it with its current relative_condition. Because Break 1 calls and_condition(0) to make the rest of the
block a no-op, the relative_condition for Break 2 becomes 0. The assignment relative_break_condition = 0 then overwrites the valid break condition from Break 1, causing the generated IR to
lose the signal to exit the loop.

Fix
The fix involves using or_condition_util to OR the new break/continue condition with any existing ones, ensuring that all possible exit paths are preserved.

1 // Fixed code in translator.cc
2 context().or_condition_util(context().relative_condition_bval(loc),
3 context().relative_break_condition, loc);

This ensures that if any break in the current context is triggered, the relative_break_condition remains true. I have applied this fix to both BreakStmt (for both for and switch bodies)
and ContinueStmt in translator.cc.

@copybara-service copybara-service bot force-pushed the test_896698882 branch 3 times, most recently from 50c0d06 to 508326e Compare April 9, 2026 17:56
Generated by Gemini. Its explanation:

✦ The PipelinedLoopOuterExit test case fails when the break on line 10929 of the test file is present because of a bug in xls/contrib/xlscc/translator.cc where
  relative_break_condition was being overwritten by subsequent break statements instead of being accumulated.

  Root Cause Analysis
  In translator.cc, within the GenerateIR_Stmt function, the handling for clang::BreakStmt and clang::ContinueStmt used simple assignment (=) for relative_break_condition and
  relative_continue_condition.

   1 // Original buggy code in translator.cc
   2 context().relative_break_condition = context().relative_condition_bval(loc);

  When a loop body contains multiple break statements, such as:

   1 if (x > 10) {
   2   if (x == 55) {
   3     exit_function = true;
   4     break; // Break 1
   5   }
   6   break;  // Break 2
   7 }
  The translator processes both break statements. When x == 55, Break 1 correctly sets the relative_break_condition. However, the translator continues to process Break 2. Although Break 2 is
  logically unreachable in C++ when Break 1 is taken, the translator still evaluates it with its current relative_condition. Because Break 1 calls and_condition(0) to make the rest of the
  block a no-op, the relative_condition for Break 2 becomes 0. The assignment relative_break_condition = 0 then overwrites the valid break condition from Break 1, causing the generated IR to
  lose the signal to exit the loop.

  Fix
  The fix involves using or_condition_util to OR the new break/continue condition with any existing ones, ensuring that all possible exit paths are preserved.

   1 // Fixed code in translator.cc
   2 context().or_condition_util(context().relative_condition_bval(loc),
   3                             context().relative_break_condition, loc);

  This ensures that if any break in the current context is triggered, the relative_break_condition remains true. I have applied this fix to both BreakStmt (for both for and switch bodies)
  and ContinueStmt in translator.cc.

PiperOrigin-RevId: 896698882
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.

0 participants