-
Notifications
You must be signed in to change notification settings - Fork 447
Fixing the Python bridge's loop and scope emission #5251
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Changes from 11 commits
8390ca2
12dfd60
6478123
9a89901
d789931
5e31634
de7fd8e
e5c1d4c
fe158c0
b877171
1f4fdae
9172c7e
b14aa1c
0ae8b28
9f0b9f1
04e3fa4
e0a9f31
a6d3100
f46c960
ea82c73
07324e6
22ba27b
7795ca5
1cdc065
10816b7
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,282 @@ | ||
| /******************************************************************************* | ||
| * Copyright (c) 2026 NVIDIA Corporation & Affiliates. * | ||
| * All rights reserved. * | ||
| * * | ||
| * This source code and the accompanying materials are made available under * | ||
| * the terms of the Apache License 2.0 which accompanies this distribution. * | ||
| ******************************************************************************/ | ||
|
|
||
| #include "PassDetails.h" | ||
| #include "cudaq/Optimizer/Dialect/CC/CCOps.h" | ||
| #include "cudaq/Optimizer/Dialect/Characteristics.h" | ||
| #include "cudaq/Optimizer/Transforms/Passes.h" | ||
| #include "mlir/Transforms/Passes.h" | ||
|
|
||
| namespace cudaq::opt { | ||
| #define GEN_PASS_DEF_LOOPPRUNEDEADARGS | ||
| #include "cudaq/Optimizer/Transforms/Passes.h.inc" | ||
| } // namespace cudaq::opt | ||
|
|
||
| #define DEBUG_TYPE "cc-loop-prune-dead-args" | ||
|
|
||
| using namespace mlir; | ||
|
|
||
| namespace { | ||
|
|
||
| using LoopCarriedSlot = std::pair<Operation *, unsigned>; | ||
|
|
||
| /// Return the slot a use passes its value into, if passing it along is all the | ||
| /// use does (a loop's initial argument, or a region terminator forwarding it | ||
| /// around the loop). | ||
| std::optional<LoopCarriedSlot> forwardedSlot(OpOperand &use) { | ||
| Operation *user = use.getOwner(); | ||
| unsigned pos = use.getOperandNumber(); | ||
| if (auto loop = dyn_cast<cudaq::cc::LoopOp>(user)) | ||
| return LoopCarriedSlot{loop.getOperation(), pos}; | ||
| auto loop = dyn_cast_or_null<cudaq::cc::LoopOp>(user->getParentOp()); | ||
| if (!loop) | ||
| return std::nullopt; | ||
| if (isa<cudaq::cc::ConditionOp>(user)) { | ||
| if (pos == 0) | ||
| return std::nullopt; | ||
| return LoopCarriedSlot{loop.getOperation(), pos - 1}; | ||
| } | ||
| if (isa<cudaq::cc::ContinueOp, cudaq::cc::BreakOp>(user)) | ||
| return LoopCarriedSlot{loop.getOperation(), pos}; | ||
| return std::nullopt; | ||
| } | ||
|
|
||
| /// Collect every value occupying slot pos of loop (the loop's result and the | ||
| /// matching entry block argument of each of its regions). | ||
| SmallVector<Value> valuesInSlot(cudaq::cc::LoopOp loop, unsigned pos) { | ||
| SmallVector<Value> values; | ||
| if (pos < loop.getNumResults()) | ||
| values.push_back(loop.getResult(pos)); | ||
| for (auto *region : loop.getRegions()) { | ||
| if (region->empty()) | ||
| continue; | ||
| Block &entry = region->front(); | ||
| if (pos < entry.getNumArguments()) | ||
| values.push_back(entry.getArgument(pos)); | ||
| } | ||
| return values; | ||
| } | ||
|
|
||
| /// The slots in dead can be removed from loop's signature only if every | ||
| /// terminator forwarding values around the loop sits directly in one of its | ||
| /// regions, and nothing reads the block arguments being dropped. | ||
| bool canEraseDeadSlots(cudaq::cc::LoopOp loop, | ||
| const llvm::SmallBitVector &dead) { | ||
| bool ok = true; | ||
| loop.getOperation()->walk([&](Operation *op) { | ||
| if (!isa<cudaq::cc::ConditionOp, cudaq::cc::BreakOp>(op) || | ||
| op->getParentOp() == loop.getOperation() || !op->getNumOperands()) | ||
| return; | ||
| // A break nested in a scope still exits this loop and carries its values. | ||
| for (auto *p = op->getParentOp(); p; p = p->getParentOp()) { | ||
| if (p == loop.getOperation()) { | ||
| ok = false; | ||
| return; | ||
| } | ||
| if (isa<cudaq::cc::LoopOp>(p)) | ||
| return; | ||
| } | ||
| }); | ||
| if (!ok) | ||
| return false; | ||
| for (auto *region : loop.getRegions()) { | ||
| if (region->empty()) | ||
| continue; | ||
| Block &entry = region->front(); | ||
| for (unsigned pos = 0, end = dead.size(); pos != end; ++pos) | ||
| if (dead[pos] && pos < entry.getNumArguments() && | ||
| !entry.getArgument(pos).use_empty()) | ||
| return false; | ||
| } | ||
| return true; | ||
| } | ||
|
|
||
| /// Rebuild loop without the slots marked in dead. | ||
| void eraseDeadSlots(cudaq::cc::LoopOp loop, const llvm::SmallBitVector &dead) { | ||
| // Trim the operands each region terminator forwards. | ||
| for (auto *region : loop.getRegions()) | ||
| for (Block &block : *region) { | ||
| if (!block.hasNoSuccessors()) | ||
| continue; | ||
| Operation *term = block.getTerminator(); | ||
| if (term->getParentOp() != loop.getOperation() || | ||
| !isa<cudaq::cc::ConditionOp, cudaq::cc::ContinueOp, | ||
| cudaq::cc::BreakOp>(term)) | ||
| continue; | ||
| unsigned offset = isa<cudaq::cc::ConditionOp>(term) ? 1 : 0; | ||
| SmallVector<Value> keep; | ||
| for (unsigned i = 0, n = term->getNumOperands(); i != n; ++i) | ||
| if (i < offset || dead.size() <= i - offset || !dead[i - offset]) | ||
| keep.push_back(term->getOperand(i)); | ||
| term->setOperands(keep); | ||
| } | ||
|
|
||
| // Drop the matching block arguments. | ||
| for (auto *region : loop.getRegions()) { | ||
| if (region->empty()) | ||
| continue; | ||
| Block &entry = region->front(); | ||
| for (int pos = dead.size() - 1; pos >= 0; --pos) | ||
| if (dead[pos] && static_cast<unsigned>(pos) < entry.getNumArguments()) | ||
| entry.eraseArgument(pos); | ||
| } | ||
|
|
||
| SmallVector<Value> newInitArgs; | ||
| SmallVector<Type> newResultTypes; | ||
| for (unsigned pos = 0, end = loop.getInitialArgs().size(); pos != end; ++pos) | ||
| if (!dead[pos]) { | ||
| newInitArgs.push_back(loop.getInitialArgs()[pos]); | ||
| if (pos < loop.getNumResults()) | ||
| newResultTypes.push_back(loop.getResultTypes()[pos]); | ||
| } | ||
|
|
||
| OpBuilder builder(loop); | ||
| auto newLoop = cudaq::cc::LoopOp::create( | ||
| builder, loop.getLoc(), newResultTypes, newInitArgs, | ||
| loop.isPostConditional(), [](OpBuilder &, Location, Region &) {}, | ||
| [](OpBuilder &, Location, Region &) {}, | ||
| /*stepBuilder=*/nullptr); | ||
| newLoop->setDiscardableAttrs(loop->getDiscardableAttrDictionary()); | ||
| newLoop.getWhileRegion().takeBody(loop.getWhileRegion()); | ||
| newLoop.getBodyRegion().takeBody(loop.getBodyRegion()); | ||
| newLoop.getStepRegion().takeBody(loop.getStepRegion()); | ||
| newLoop.getElseRegion().takeBody(loop.getElseRegion()); | ||
|
|
||
| unsigned newPos = 0; | ||
| for (unsigned pos = 0, end = loop.getNumResults(); pos != end; ++pos) | ||
| if (!dead[pos]) | ||
| loop.getResult(pos).replaceAllUsesWith(newLoop.getResult(newPos++)); | ||
| loop.erase(); | ||
| } | ||
|
|
||
| void pruneDeadLoopCarriedValues(func::FuncOp func) { | ||
| SmallVector<cudaq::cc::LoopOp> loops; | ||
| func.walk([&](cudaq::cc::LoopOp loop) { loops.push_back(loop); }); | ||
| if (loops.empty()) | ||
| return; | ||
|
|
||
| // Reaching the loop-carried values means going through the arithmetic that | ||
| // computes them, and that arithmetic reads the loop's own block arguments. | ||
| SmallVector<Value> candidates; | ||
| auto isPrunableComputation = [](Operation *op) { | ||
| return op->getNumRegions() == 0 && !op->hasTrait<OpTrait::IsTerminator>() && | ||
| !cudaq::opt::hasQuantum(*op) && isMemoryEffectFree(op); | ||
| }; | ||
| for (auto loop : loops) | ||
| for (unsigned pos = 0, end = loop.getInitialArgs().size(); pos != end; | ||
| ++pos) | ||
| llvm::append_range(candidates, valuesInSlot(loop, pos)); | ||
| func.walk([&](Operation *op) { | ||
| if (isPrunableComputation(op)) | ||
| llvm::append_range(candidates, op->getResults()); | ||
| }); | ||
|
|
||
| // Start from the assumption that every candidate is dead and propagate | ||
| // liveness until it stops spreading. A value is live if | ||
| // 1. it has an opaque use | ||
| // 2. it is forwarded into a slot already known to be live | ||
| // 3. it feeds a computation whose own result is already known to be live | ||
| // Liveness only ever grows and the candidate set is finite, so this | ||
| // terminates. | ||
| DenseSet<Value> live; | ||
| auto slotIsLive = [&](LoopCarriedSlot slot) { | ||
| auto loop = cast<cudaq::cc::LoopOp>(slot.first); | ||
| return llvm::any_of(valuesInSlot(loop, slot.second), | ||
| [&](Value val) { return live.count(val); }); | ||
| }; | ||
| auto hasLiveUse = [&](Value val) { | ||
| for (OpOperand &use : val.getUses()) { | ||
| if (auto forwarded = forwardedSlot(use)) { | ||
| if (slotIsLive(*forwarded)) | ||
| return true; | ||
| continue; | ||
| } | ||
| Operation *user = use.getOwner(); | ||
| if (!isPrunableComputation(user)) | ||
| return true; | ||
| if (llvm::any_of(user->getResults(), | ||
| [&](Value res) { return live.count(res); })) | ||
| return true; | ||
| } | ||
| return false; | ||
| }; | ||
| for (bool changed = true; changed;) { | ||
| changed = false; | ||
| for (Value val : candidates) | ||
| if (!live.count(val) && hasLiveUse(val)) { | ||
| live.insert(val); | ||
| changed = true; | ||
| } | ||
| } | ||
|
|
||
| // Short-circuit each dead slot with the loop's initial value for that slot. | ||
| // The initial value is an operand of the loop, so it dominates every use we | ||
| // rewrite. | ||
| for (auto loop : loops) | ||
| for (auto [pos, initialArg] : llvm::enumerate(loop.getInitialArgs())) { | ||
| if (slotIsLive(LoopCarriedSlot{loop.getOperation(), pos})) | ||
| continue; | ||
| for (auto *region : loop.getRegions()) | ||
| for (Block &block : *region) { | ||
| if (!block.hasNoSuccessors()) | ||
| continue; | ||
| Operation *term = block.getTerminator(); | ||
| if (term->getParentOp() != loop.getOperation()) | ||
| continue; | ||
| unsigned operandPos = | ||
| isa<cudaq::cc::ConditionOp>(term) ? pos + 1 : pos; | ||
| if (isa<cudaq::cc::ConditionOp, cudaq::cc::ContinueOp, | ||
| cudaq::cc::BreakOp>(term) && | ||
| operandPos < term->getNumOperands()) | ||
| term->setOperand(operandPos, initialArg); | ||
| } | ||
| } | ||
|
|
||
| // Drop each dead slot from the loop's signature. Erasing an inner loop's | ||
| // slots is what frees up the enclosing loop's, so keep going until nothing | ||
| // more can be dropped. | ||
| for (bool changed = true; changed;) { | ||
| changed = false; | ||
| SmallVector<cudaq::cc::LoopOp> current; | ||
| func.walk([&](cudaq::cc::LoopOp loop) { current.push_back(loop); }); | ||
| for (auto loop : llvm::reverse(current)) { | ||
| llvm::SmallBitVector dead(loop.getInitialArgs().size()); | ||
| for (unsigned pos = 0, end = dead.size(); pos != end; ++pos) | ||
| if (!slotIsLive(LoopCarriedSlot{loop.getOperation(), pos})) | ||
| dead.set(pos); | ||
| if (dead.none() || !canEraseDeadSlots(loop, dead)) | ||
| continue; | ||
| eraseDeadSlots(loop, dead); | ||
| changed = true; | ||
| break; | ||
| } | ||
| } | ||
|
|
||
| // Delete the classical computations that just went dead. | ||
| for (bool erased = true; erased;) { | ||
| erased = false; | ||
| SmallVector<Operation *> deadOps; | ||
| func.walk([&](Operation *op) { | ||
| if (!cudaq::opt::hasQuantum(*op) && isOpTriviallyDead(op)) | ||
| deadOps.push_back(op); | ||
| }); | ||
| for (auto *op : deadOps) { | ||
| op->erase(); | ||
| erased = true; | ||
| } | ||
| } | ||
| } | ||
|
|
||
| class LoopPruneDeadArgsPass | ||
| : public cudaq::opt::impl::LoopPruneDeadArgsBase<LoopPruneDeadArgsPass> { | ||
| public: | ||
| using LoopPruneDeadArgsBase::LoopPruneDeadArgsBase; | ||
|
|
||
| void runOnOperation() override { pruneDeadLoopCarriedValues(getOperation()); } | ||
| }; | ||
| } // namespace |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -826,10 +826,23 @@ class RegionDataFlow { | |
| getBinding(block, std::get<0>(info)) == std::get<1>(info)) | ||
| addBinding(block, std::get<0>(info), newReg); | ||
| user->replaceUsesOfWith(std::get<1>(info), newReg); | ||
| // Other variables bound to this value follow it to the block | ||
| // argument, so `x = i` in a loop body gets this iteration's value. | ||
| updateBindingsOfValue(block, std::get<1>(info), newReg); | ||
|
sacpis marked this conversation as resolved.
Outdated
|
||
| } | ||
| } | ||
| } | ||
|
|
||
| /// Point any binding in block that still refers to oldVal at newVal. | ||
| void updateBindingsOfValue(Block *block, Value oldVal, Value newVal) { | ||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Still looks wrongheaded.
Collaborator
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Fixed in 07324e6. The sweep is gone. The binding update is now anchored to the store being rewritten. If the user we're rewriting is a Note: This one is a miscompile on main today.
Collaborator
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Here is the qke after running Please see With this fix on this branch, we get With this code The kernel above set j to 2. This fix gives the correct output as Hope this helps. |
||
| auto iter = rMap.find(block); | ||
| if (iter == rMap.end()) | ||
| return; | ||
| for (auto &binding : iter->second) | ||
| if (binding.second == oldVal) | ||
| binding.second = newVal; | ||
| } | ||
|
|
||
| /// Track the memory reference \p mr as being live-out of the parent | ||
| /// operation. (\p parent is passed for the assertion check only.) | ||
| void addLiveOutOfParent(Operation *parent, MemRef mr) { | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This was going to be removed, eh?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This pass is what lets two workarounds on main go away. Both are from #5233.
allocasinking invisit_For/__analyzeLoopLocalTargets(the bare cc.alloca in the loop body)isNestedInLoopguard inVariableCoalesce.cpp, which refuses to raise anyallocanested in a cc.loopThis branch deletes both and prunes the dead loop-carried values in the optimizer instead. #5223 needs one of the three. With the pass disabled and nothing else changed,
test_5223.pyfails with the original 'arith.addi' op control-flow def-use not reversible.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
OK.
So the isNestedInLoop stuff to move cc.alloca around is misplaced and possibly redundant with what stack-frame-prealloc does.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Totally agreed on the front end, and that's what this PR does. The bridge emits
cc.scopeforif/elseand loop bodies now, and the sinking that produced the barecc.allocais gone.The
isNestedInLoopguard was a consequence of that same front end bug.variable-coalescesays itrelies on the correct and proper construction and use of cc.scope ops,and the bridge emitted none, so #5233 guarded the pass instead of fixing the IR. With scopes emitted the guard is not needed, andcoalesce.qke's f2 goes back to 2 slots from 4.stack-frame-preallocis downstream of this (it runs inaddLowerToCFGAndCleanupafter lowering to CFG, whereasapply-op-specializationruns on the high-level IR, so it cannot cover this case).