[MLIR][SCF] fix loop pipelining pass use of uninitialized value #146991

Hyffer · 2025-07-04T02:43:06Z

github-actions · 2025-07-04T02:43:24Z

Thank you for submitting a Pull Request (PR) to the LLVM Project!

This PR will be automatically labeled and the relevant teams will be notified.

If you wish to, you can add reviewers by using the "Reviewers" section on this page.

If this is not working for you, it is probably because you do not have write permissions for the repository. In which case you can instead tag reviewers by name in a comment by using @ followed by their GitHub username.

If you have received no comments on your PR for a week, you can request a review by "ping"ing the PR by adding a comment “Ping”. The common courtesy "ping" rate is once a week. Please remember that you are asking for valuable time from other developers.

If you have further questions, they may be answered by the LLVM GitHub User Guide.

You can also ask questions in a comment on this PR, on the LLVM Discord or on the forums.

llvmbot · 2025-07-04T02:43:52Z

@llvm/pr-subscribers-mlir-scf

@llvm/pr-subscribers-mlir

Author: Hu Yufan (Hyffer)

Changes

fix issue #146990

Full diff: https://github.com/llvm/llvm-project/pull/146991.diff

1 Files Affected:

(modified) mlir/lib/Dialect/SCF/Transforms/LoopPipelining.cpp (+14-13)

diff --git a/mlir/lib/Dialect/SCF/Transforms/LoopPipelining.cpp b/mlir/lib/Dialect/SCF/Transforms/LoopPipelining.cpp
index 4aacbe739ca5d..018ac729de8f9 100644
--- a/mlir/lib/Dialect/SCF/Transforms/LoopPipelining.cpp
+++ b/mlir/lib/Dialect/SCF/Transforms/LoopPipelining.cpp
@@ -106,6 +106,20 @@ bool LoopPipelinerInternal::initializeLoopInfo(
   lb = forOp.getLowerBound();
   step = forOp.getStep();
 
+  std::vector<std::pair<Operation *, unsigned>> schedule;
+  options.getScheduleFn(forOp, schedule);
+  if (schedule.empty()) {
+    LDBG("--empty schedule -> BAIL");
+    return false;
+  }
+
+  opOrder.reserve(schedule.size());
+  for (auto &opSchedule : schedule) {
+    maxStage = std::max(maxStage, opSchedule.second);
+    stages[opSchedule.first] = opSchedule.second;
+    opOrder.push_back(opSchedule.first);
+  }
+
   dynamicLoop = true;
   auto upperBoundCst = getConstantIntValue(ub);
   auto lowerBoundCst = getConstantIntValue(lb);
@@ -137,19 +151,6 @@ bool LoopPipelinerInternal::initializeLoopInfo(
     LDBG("--no epilogue or predicate set -> BAIL");
     return false;
   }
-  std::vector<std::pair<Operation *, unsigned>> schedule;
-  options.getScheduleFn(forOp, schedule);
-  if (schedule.empty()) {
-    LDBG("--empty schedule -> BAIL");
-    return false;
-  }
-
-  opOrder.reserve(schedule.size());
-  for (auto &opSchedule : schedule) {
-    maxStage = std::max(maxStage, opSchedule.second);
-    stages[opSchedule.first] = opSchedule.second;
-    opOrder.push_back(opSchedule.first);
-  }
 
   // All operations need to have a stage.
   for (Operation &op : forOp.getBody()->without_terminator()) {

Hyffer · 2025-07-04T02:49:50Z

This is my first time contribute to llvm, I have went through contributing guide. And I have followed https://mlir.llvm.org/getting_started/TestingGuide/, test results:

********************
Failed Tests (3):
  MLIR :: Dialect/SCF/transform-ops.mlir
  MLIR-Unit :: Target/LLVM/./MLIRTargetLLVMTests/MLIRTargetLLVMROCDL/GetELFMetadata
  MLIR-Unit :: Target/LLVM/./MLIRTargetLLVMTests/MLIRTargetLLVMROCDL/SerializeROCDLToBinary


Testing Time: 42.47s

Total Discovered Tests: 3332
  Skipped          :    1 (0.03%)
  Unsupported      :  458 (13.75%)
  Passed           : 2869 (86.10%)
  Expectedly Failed:    1 (0.03%)
  Failed           :    3 (0.09%)

It seems those three just fail on main branch.

ThomasRaoux · 2025-07-04T03:06:02Z

Thanks for fixing this. Most likely the lit tests in Dialect/SCF/transform-ops.mlir needs an update.
You can find some info about lit test here: https://mlir.llvm.org/getting_started/TestingGuide/#lit-and-filecheck-tests

Hyffer · 2025-07-04T08:27:06Z

The two failed cases in Dialect/SCF/transform-ops.mlir are both corner cases that numIteration == maxStage. In those cases, loops are able to properly pipelined, with a pipeline kernel that iterates for zero time, looks like:

S0(0)                        
S0(1) S1(0)                  
scf.for ... to ... {            <-- which will iterates for zero time
  S0(I+2) S1(I+1) S2(I)      
}
S1(N) S2(N-1)                
S2(N)

Test Dialect/SCF/transform-ops.mlir itself is correct. I have updated the checking condition from numIteration > maxStage to numIteration >= maxStage in code. @ThomasRaoux could you have a review on that?

********************
Failed Tests (2):
  MLIR-Unit :: Target/LLVM/./MLIRTargetLLVMTests/MLIRTargetLLVMROCDL/GetELFMetadata
  MLIR-Unit :: Target/LLVM/./MLIRTargetLLVMTests/MLIRTargetLLVMROCDL/SerializeROCDLToBinary


Testing Time: 12.77s

Total Discovered Tests: 3332
  Skipped          :    1 (0.03%)
  Unsupported      :  458 (13.75%)
  Passed           : 2870 (86.13%)
  Expectedly Failed:    1 (0.03%)
  Failed           :    2 (0.06%)

Copilot

Pull Request Overview

This PR fixes the use of an uninitialized scheduling value in the loop pipelining pass by moving and deduplicating schedule initialization, and tightens the iteration-vs-stage comparison.

Moved schedule fetching and processing to the top of initializeLoopInfo and removed the duplicate block.
Updated the boundary check from > to >= when comparing numIteration and maxStage.

Comments suppressed due to low confidence (1)

mlir/lib/Dialect/SCF/Transforms/LoopPipelining.cpp:141

Add a unit test case covering the boundary condition where numIteration == maxStage to verify that dynamicLoop is correctly set to false in this scenario.

    if (numIteration >= maxStage) {

Copilot · 2025-07-04T11:47:46Z

mlir/lib/Dialect/SCF/Transforms/LoopPipelining.cpp

Before reserving and populating opOrder and stages, ensure both containers are cleared (e.g., opOrder.clear(); stages.clear();) to avoid carrying over any stale entries when initializeLoopInfo is called multiple times.

ThomasRaoux · 2025-07-05T01:24:13Z

The two failed cases in Dialect/SCF/transform-ops.mlir are both corner cases that numIteration == maxStage. In those cases, loops are able to properly pipelined, with a pipeline kernel that iterates for zero time, looks like:
S0(0)                        
S0(1) S1(0)                  
scf.for ... to ... {            <-- which will iterates for zero time
  S0(I+2) S1(I+1) S2(I)      
}
S1(N) S2(N-1)                
S2(N)                        
Test Dialect/SCF/transform-ops.mlir itself is correct. I have updated the checking condition from numIteration > maxStage to numIteration >= maxStage in code. @ThomasRaoux could you have a review on that?
********************
Failed Tests (2):
  MLIR-Unit :: Target/LLVM/./MLIRTargetLLVMTests/MLIRTargetLLVMROCDL/GetELFMetadata
  MLIR-Unit :: Target/LLVM/./MLIRTargetLLVMTests/MLIRTargetLLVMROCDL/SerializeROCDLToBinary


Testing Time: 12.77s

Total Discovered Tests: 3332
  Skipped          :    1 (0.03%)
  Unsupported      :  458 (13.75%)
  Passed           : 2870 (86.13%)
  Expectedly Failed:    1 (0.03%)
  Failed           :    2 (0.06%)

makes sense. Could you add a lit test for the case it changes (when we detect dynamicLoop=false)?

Hyffer · 2025-07-05T04:54:25Z

makes sense. Could you add a lit test for the case it changes (when we detect dynamicLoop=false)?

Sorry but I don't get the point. Currently most test cases in Dialect/SCF/transform-opt.mlir and Dialect/SCF/loop-pipelining.mlir satisfy numIteration >= maxStage thus dynamicLoop = false.

I added a test case that checks when a static loop does not satisfy numIteration >= maxStage, if I understand correctly.

ThomasRaoux · 2025-07-05T18:34:23Z

makes sense. Could you add a lit test for the case it changes (when we detect dynamicLoop=false)?

Sorry but I don't get the point. Currently most test cases in Dialect/SCF/transform-opt.mlir and Dialect/SCF/loop-pipelining.mlir satisfy numIteration >= maxStage thus dynamicLoop = false.

I added a test case that checks when a static loop does not satisfy numIteration >= maxStage, if I understand correctly.

right, sorry that's what I meant

ThomasRaoux

LGTM

github-actions · 2025-07-05T19:21:48Z

@Hyffer Congratulations on having your first Pull Request (PR) merged into the LLVM Project!

Your changes will be combined with recent changes from other authors, then tested by our build bots. If there is a problem with a build, you may receive a report in an email or a comment on this PR.

Please check whether problems have been caused by your change specifically, as the builds can include changes from many authors. It is not uncommon for your change to be included in a build that fails due to someone else's changes, or infrastructure issues.

How to do this, and the rest of the post-merge process, is covered in detail here.

If your change does cause a problem, it may be reverted, or you can revert it yourself. This is a normal part of LLVM development. You can fix your changes and open a new PR to merge them again.

If you don't get any reports, no action is required from you. Your changes are working as expected, well done!

llvmbot added mlir mlir:scf labels Jul 4, 2025

Hyffer force-pushed the users/hyffer/mlir-pipeliningpass-fix branch 2 times, most recently from 130d637 to 09e8fca Compare July 4, 2025 08:11

joker-eph requested review from ThomasRaoux and Copilot July 4, 2025 11:47

Copilot AI reviewed Jul 4, 2025

View reviewed changes

[MLIR][SCF] fix pipelining pass use of uninitialized value

f537b8e

Hyffer force-pushed the users/hyffer/mlir-pipeliningpass-fix branch from fc6f29d to f537b8e Compare July 5, 2025 04:54

ThomasRaoux approved these changes Jul 5, 2025

View reviewed changes

ThomasRaoux linked an issue Jul 5, 2025 that may be closed by this pull request

[MLIR][SCF] pipelining pass use of uninitialized value #146990

Closed

ThomasRaoux merged commit 945ed2e into llvm:main Jul 5, 2025
9 checks passed

[MLIR][SCF] fix loop pipelining pass use of uninitialized value #146991

[MLIR][SCF] fix loop pipelining pass use of uninitialized value #146991

Uh oh!

Conversation

Hyffer commented Jul 4, 2025

Uh oh!

github-actions bot commented Jul 4, 2025

Uh oh!

llvmbot commented Jul 4, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Hyffer commented Jul 4, 2025

Uh oh!

ThomasRaoux commented Jul 4, 2025

Uh oh!

Hyffer commented Jul 4, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull Request Overview

Uh oh!

Copilot AI Jul 4, 2025

Choose a reason for hiding this comment

Uh oh!

ThomasRaoux commented Jul 5, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Hyffer commented Jul 5, 2025

Uh oh!

ThomasRaoux commented Jul 5, 2025

Uh oh!

ThomasRaoux left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

github-actions bot commented Jul 5, 2025

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

llvmbot commented Jul 4, 2025 •

edited

Loading

Hyffer commented Jul 4, 2025 •

edited

Loading

ThomasRaoux commented Jul 5, 2025 •

edited

Loading