Skip to content

Update canonicalization of cc.if - #5303

Open
atgeller wants to merge 1 commit into
NVIDIA:mainfrom
atgeller:if_canon
Open

Update canonicalization of cc.if#5303
atgeller wants to merge 1 commit into
NVIDIA:mainfrom
atgeller:if_canon

Conversation

@atgeller

Copy link
Copy Markdown
Collaborator

No description provided.

Signed-off-by: Adam Geller <adgeller@nvidia.com>
@github-actions

Copy link
Copy Markdown

CI Summary (push) — ✅ passed

Run #33046195180 · ✅ 7 · ⏩ 7 · ❌ 0 · ⛔ 0

Top-level jobs (14)
Job Result
binaries ⏩ skipped
build_and_test ✅ success
changes ✅ success
config_devdeps ✅ success
config_source_build ⏩ skipped
config_wheeldeps ✅ success
devdeps ✅ success
docker_image ⏩ skipped
gen_code_coverage ⏩ skipped
metadata ✅ success
python_metapackages ⏩ skipped
python_wheels ⏩ skipped
source_build ⏩ skipped
wheeldeps ✅ success
⏩ Skipped jobs (7) — intentionally skipped on PR builds; run on merge_group / workflow_dispatch
Job
binaries
config_source_build
docker_image
gen_code_coverage
python_metapackages
python_wheels
source_build
All sub-jobs (43) — every matrix leg, with links
Job Status Link
Build and test (amd64, gcc12, openmpi) / Dev environment (Debug) ✅ success view
Build and test (amd64, gcc12, openmpi) / Dev environment (Python) ✅ success view
Build and test (amd64, llvm, openmpi) / Dev environment (Debug) ✅ success view
Build and test (amd64, llvm, openmpi) / Dev environment (Python) ✅ success view
Build and test (arm64, llvm, openmpi) / Dev environment (Debug) ✅ success view
Build and test (arm64, llvm, openmpi) / Dev environment (Python) ✅ success view
CI Summary ❔ in_progress view
Check for stable CUDA-Q changes ✅ success view
Configure build (devdeps) ✅ success view
Configure build (source_build) ⏩ skipped view
Configure build (wheeldeps) ✅ success view
Create CUDA Quantum installer ⏩ skipped view
Create Docker images ⏩ skipped view
Create Python metapackages ⏩ skipped view
Create Python wheels ⏩ skipped view
Gen code coverage ⏩ skipped view
Load dependencies (amd64, gcc12) / Caching ✅ success view
Load dependencies (amd64, gcc12) / Finalize ✅ success view
Load dependencies (amd64, gcc12) / Metadata ✅ success view
Load dependencies (amd64, llvm) / Caching ✅ success view
Load dependencies (amd64, llvm) / Finalize ✅ success view
Load dependencies (amd64, llvm) / Metadata ✅ success view
Load dependencies (arm64, gcc12) / Caching ✅ success view
Load dependencies (arm64, gcc12) / Finalize ✅ success view
Load dependencies (arm64, gcc12) / Metadata ✅ success view
Load dependencies (arm64, llvm) / Caching ✅ success view
Load dependencies (arm64, llvm) / Finalize ✅ success view
Load dependencies (arm64, llvm) / Metadata ✅ success view
Load source build cache ⏩ skipped view
Load wheel dependencies (amd64, 12.6) / Caching ✅ success view
Load wheel dependencies (amd64, 12.6) / Finalize ✅ success view
Load wheel dependencies (amd64, 12.6) / Metadata ✅ success view
Load wheel dependencies (amd64, 13.0) / Caching ✅ success view
Load wheel dependencies (amd64, 13.0) / Finalize ✅ success view
Load wheel dependencies (amd64, 13.0) / Metadata ✅ success view
Load wheel dependencies (arm64, 12.6) / Caching ✅ success view
Load wheel dependencies (arm64, 12.6) / Finalize ✅ success view
Load wheel dependencies (arm64, 12.6) / Metadata ✅ success view
Load wheel dependencies (arm64, 13.0) / Caching ✅ success view
Load wheel dependencies (arm64, 13.0) / Finalize ✅ success view
Load wheel dependencies (arm64, 13.0) / Metadata ✅ success view
Prepare cache clean-up ❔ in_progress view
Retrieve PR info ✅ success view
✅ Required checks (6/6) — declared in .github/required-checks.yml for push
Required check Status Link
Build and test (amd64, llvm, openmpi) / Dev environment (Debug) ✅ success view
Build and test (amd64, llvm, openmpi) / Dev environment (Python) ✅ success view
Build and test (arm64, llvm, openmpi) / Dev environment (Debug) ✅ success view
Build and test (arm64, llvm, openmpi) / Dev environment (Python) ✅ success view
Build and test (amd64, gcc12, openmpi) / Dev environment (Debug) ✅ success view
Build and test (amd64, gcc12, openmpi) / Dev environment (Python) ✅ success view

@sacpis sacpis left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Overall LGTM. Thanks @atgeller. Left a comment.

Comment on lines +2551 to +2575
// General case: the region has multiple exits, so split the parent block at
// the cc.if and stitch the region in with branches.
auto *ifBlock = rewriter.getInsertionBlock();
auto *splitBlock =
rewriter.splitBlock(ifBlock, rewriter.getInsertionPoint());
Block *succBlock = splitBlock;
if (ifOp.getNumResults()) {
succBlock = rewriter.createBlock(
splitBlock, ifOp.getResultTypes(),
SmallVector<Location>(ifOp.getNumResults(), loc));
cf::BranchOp::create(rewriter, loc, splitBlock);
}
rewriter.replaceOpWithNewOp<cudaq::cc::IfOp>(
ifOp, ifOp.getResultTypes(), truth,
[&](OpBuilder &, Location, Region &region) {
region.takeBody(*newRegion);
});
auto *entryBlock = &region.front();
for (auto &block : region)
if (auto contOp =
dyn_cast<cudaq::cc::ContinueOp>(block.getTerminator())) {
rewriter.setInsertionPointToEnd(&block);
rewriter.replaceOpWithNewOp<cf::BranchOp>(contOp, succBlock,
contOp.getOperands());
}
rewriter.inlineRegionBefore(region, succBlock);
rewriter.setInsertionPointToEnd(ifBlock);
cf::BranchOp::create(rewriter, loc, entryBlock, ifOp.getLinearArgs());
rewriter.replaceOp(ifOp, succBlock->getArguments());
return success();

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

If we fold a multi exit cc.if inside a loop's while or step region it produces invalid IR and the canonicalizer errors out.

I tried running cudaq-opt -canonicalize on the following mlir

func.func private @side() -> i1
func.func @in_while(%arg: i1) {
  %c = arith.constant true
  cc.loop while {
    cc.if (%c) {
      cf.cond_br %arg, ^bb1, ^bb2
    ^bb1:
      %z = func.call @side() : () -> i1
      cc.continue
    ^bb2:
      cc.continue
    }
    %p = func.call @side() : () -> i1
    cc.condition %p
  } do {
    %y = func.call @side() : () -> i1
    cc.continue
  }
  return
}

It gives me the error as

error: 'cc.loop' op region #0 ('whileRegion') failed to verify constraint: region with 1 blocks

I think before taking the multi exit path, we should bail out when the region holding the cc.if cannot take extra blocks. The single block path do not need such a guard.

@schweitzpgi schweitzpgi left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Thanks for the renovations. A couple of quick comments.

PatternRewriter &rewriter) const override {
auto cond = ifOp.getCondition();
auto con = cond.getDefiningOp<arith::ConstantIntOp>();
auto con = ifOp.getCondition().getDefiningOp<arith::ConstantIntOp>();

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Let's use cudaq::opt::factory::maybeValueOfIntConstant

rewriter.inlineBlockBefore(&entryBlock, ifOp, ifOp.getLinearArgs());
// Simple case: a single block, which must end with a cc.continue. Splice
// the block in place of the cc.if.
if (region.hasOneBlock()) {

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Can we splice the entire region, rewrite the cc.continue operations to cf.br to a new block with block arguments and then rewrite all uses of the cc.if values to use those block arguments? That should handle the CFG inside the then/else region cases.

It should be the same, effectively, as what lower-to-cfg does. MIght be a refactor opportunity?

}
};

struct KillIfWithNoOpArms : public OpRewritePattern<cudaq::cc::IfOp> {

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Good idea.

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.

3 participants