Skip to content
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

[CIR] Add -cir-mlir-scf-prepare to simplify lowering to SCF #604

Merged
merged 3 commits into from
May 24, 2024

Conversation

ShivaChen
Copy link
Contributor

@ShivaChen ShivaChen commented May 15, 2024

This commit introduces SCFPreparePass to

  1. Canonicalize IV to LHS of loop comparison
    For example, transfer cir.cmp(gt, %bound, %IV) to cir.cmp(lt, %IV, %bound). So we could use RHS as boundary and use lt to determine it's an upper bound.
  2. Hoist loop invariant operations in condition block out of loop.
    The condition block may be generated as following which contains the operations produced upper bound.
    SCF for loop required loop boundary as input operands. So we might need to hoist the boundary operations out of loop.
      cir.for : cond {
        %4 = cir.load %2 : !cir.ptr<!s32i>, !s32i
        %5 = cir.const #cir.int<100> : !s32i       <- upper bound
        %6 = cir.cmp(lt, %4, %5) : !s32i, !s32i
        %7 = cir.cast(int_to_bool, %6 : !s32i), !cir.boo
        cir.condition(%7
       } body {

@bcardosolopes
Copy link
Member

Thanks for the PR. Can you share a bit more of what are your plans for using this? I'm assuming this is to make it easier to lower to MLIR, but I'd be great to have more details in the description.

Also, this probably makes more sense elsewhere, lowerprepare is more about additional lowering, not really canonicalization. It could be another pass but an option I'd prefer is to rename MergeCleanups to something a bit more generic and implement it there. It'd also be great if it's easier to integrate later on as part of MLIR generic canonicalization (which we cannot yet use because it's too aggressive). @orbiri any opinions here?

@ShivaChen
Copy link
Contributor Author

Thanks for the PR. Can you share a bit more of what are your plans for using this? I'm assuming this is to make it easier to lower to MLIR, but I'd be great to have more details in the description.

Sorry for missing more details. I have updated the PR description.

Also, this probably makes more sense elsewhere, lowerprepare is more about additional lowering, not really canonicalization. It could be another pass but an option I'd prefer is to rename MergeCleanups to something a bit more generic and implement it there. It'd also be great if it's easier to integrate later on as part of MLIR generic canonicalization (which we cannot yet use because it's too aggressive). @orbiri any opinions here?

Agree that elsewhere would make more sense. I will move to MergeCleanups and perhaps update the pass name when we have agreement.

@ShivaChen
Copy link
Contributor Author

Thanks for the PR. Can you share a bit more of what are your plans for using this? I'm assuming this is to make it easier to lower to MLIR, but I'd be great to have more details in the description.

Sorry for missing more details. I have updated the PR description.

Also, this probably makes more sense elsewhere, lowerprepare is more about additional lowering, not really canonicalization. It could be another pass but an option I'd prefer is to rename MergeCleanups to something a bit more generic and implement it there. It'd also be great if it's easier to integrate later on as part of MLIR generic canonicalization (which we cannot yet use because it's too aggressive). @orbiri any opinions here?

Agree that elsewhere would make more sense. I will move to MergeCleanups and perhaps update the pass name when we have agreement.

I was separating the canonicalization for SCF loop to this PR because it's more complicated when implementing in LowerCIRToMLIR. Given that we decided to move loop lowering before LowerCIRToMLIR in #605 (comment), perhaps another option could be moving the canonicalization to the pass if the canonicalization mere benefit for MLIR path?

@bcardosolopes
Copy link
Member

I was separating the canonicalization for SCF loop to this PR because it's more complicated when implementing in LowerCIRToMLIR.

Yea, sounds like the right call to me!

Given that we decided to move loop lowering before LowerCIRToMLIR in #605 (comment), perhaps another option could be moving the canonicalization to the pass if the canonicalization mere benefit for MLIR path?

Not sure I understood, so correct if I'm wrong, but you are suggesting we split this into another pass entirely because it might not be profitable for the non-MLIR lowering path, is that right? If so, sounds good to me too. MergeCleanups will run for both pipelines and you are right, it'd just add compile time for the pure LLVM pipeline - if in the future we see a need, we can enable it for the LLVM pipeline.

@ShivaChen
Copy link
Contributor Author

I was separating the canonicalization for SCF loop to this PR because it's more complicated when implementing in LowerCIRToMLIR.

Yea, sounds like the right call to me!

Given that we decided to move loop lowering before LowerCIRToMLIR in #605 (comment), perhaps another option could be moving the canonicalization to the pass if the canonicalization mere benefit for MLIR path?

Not sure I understood, so correct if I'm wrong, but you are suggesting we split this into another pass entirely because it might not be profitable for the non-MLIR lowering path, is that right? If so, sounds good to me too. MergeCleanups will run for both pipelines and you are right, it'd just add compile time for the pure LLVM pipeline - if in the future we see a need, we can enable it for the LLVM pipeline.

Yes, I was thinking split it to a new pass. Is there new pass name you would prefer?

@bcardosolopes
Copy link
Member

Yes, I was thinking split it to a new pass. Is there new pass name you would prefer?

I'm pretty open, how about -cir-mlir-scf-prepare?

This commit introduces SCFPreparePass to

1) Canonicalize IV to LHS of loop comparison

   For example, transfer cir.cmp(gt, %bound, %IV) to cir.cmp(lt, %IV, %bound).
   So we could use RHS as boundary and use lt to determine it's an upper bound.

2) Hoist loop invariant operations in condition block out of loop

   The condition block may be generated as following which contains the
   operations produced upper bound.

   SCF for loop required loop boundary as input operands. So we need to
   hoist the boundary operations out of loop.

   cir.for : cond {
     %4 = cir.load %2 : !cir.ptr<!s32i>, !s32i
     %5 = cir.const #cir.int<100> : !s32i       <- upper bound
     %6 = cir.cmp(lt, %4, %5) : !s32i, !s32i
     %7 = cir.cast(int_to_bool, %6 : !s32i), !cir.bool
     cir.condition(%7
  } body {
@ShivaChen
Copy link
Contributor Author

Yes, I was thinking split it to a new pass. Is there new pass name you would prefer?

I'm pretty open, how about -cir-mlir-scf-prepare?

Sounds good to me. Thanks.

@ShivaChen ShivaChen changed the title [CIR][LoweringPrepare] Add lowerForOp to simplify lowering to MLIR [CIR] Add -cir-mlir-scf-prepare to simplify lowering to SCF May 22, 2024
Copy link
Member

@bcardosolopes bcardosolopes left a comment

Choose a reason for hiding this comment

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

Awesome, LGTM with minor nits

clang/include/clang/CIR/Dialect/Passes.td Outdated Show resolved Hide resolved
clang/lib/CIR/CodeGen/CIRPasses.cpp Show resolved Hide resolved
@bcardosolopes bcardosolopes merged commit 957c8d2 into llvm:main May 24, 2024
6 checks passed
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.

None yet

2 participants