[CIR][ThroughMLIR] Support lowering cir.condition and cir.while to scf.condition, scf.while #636
Conversation
GaoXiangYa
changed the title
| // RUN: FileCheck %s --input-file %t.mlir | ||
|
|
||
| !s32i = !cir.int<s, 32> | ||
| module { |
There was a problem hiding this comment.
Please either add a comment with the C/C++ code we could use to regen this test in case CIR changes significantly or add a test coming from C/C++ source.
There was a problem hiding this comment.
Ok, I will add some comment and test case
There was a problem hiding this comment.
Hello, I add a test coming from C source , can you review it?
|
@ShivaChen can you review this and let us know if anything should be done regarding the loop canonicalization pass? |
Sure, I'll take a look. |
|
Hello, if you have review the code, and agree with me, I want to try to move the |
Yes, I agree LowerCIRLoopToSCF.cpp could be right place to go. Thanks! |
It seems cir.while to scf.while have one to one mapping. |
| #include "llvm/Analysis/RegionInfo.h" | ||
| #include "llvm/IR/Type.h" | ||
| #include "llvm/IR/Value.h" | ||
| #include "llvm/Support/Casting.h" |
There was a problem hiding this comment.
nit: does the new adding include lines are required?
There was a problem hiding this comment.
These files are generated by clangd’s auto-completion, and I believe they are not necessary
There was a problem hiding this comment.
Hello, I have moved the CIRWhileOpLowering to LoweringSCFLoop.cpp , can you review it?
There was a problem hiding this comment.
Could we remove the un-required including lines?
| #include "llvm/Analysis/RegionInfo.h" | ||
| #include "llvm/IR/Type.h" | ||
| #include "llvm/IR/Value.h" | ||
| #include "llvm/Support/Casting.h" |
There was a problem hiding this comment.
Could we remove the un-required including lines?
|
Do you mean I add a SCFWhileLoop class ? And I will delete the header file. @ShivaChen |
I think the way you implement CIRWhileOpLowering in e882b20 |
| @@ -965,6 +973,27 @@ class CIRYieldOpLowering | |||
| } | |||
| }; | |||
|
|
|||
| class CIRConditionOpLowering | |||
There was a problem hiding this comment.
It seems ConditionOp only be used for loops. Should we move to LowerCIRLoopToSCF.cpp?
There was a problem hiding this comment.
I move the CIRConditionOpLowering to LoweriCIRLoopToSCF
|
Could you remove the changes of deleting empty lines and include headers in LowerCIRToMLIR.cpp? |
|
Sorry , comparing with the old LowerCIRToMLIR.cpp , I don`t find some empty lines, and I have already deleted the un-used header files. @ShivaChen |
It seems I could see when clicking File Chagned on the web page but it could be a nit-picking. |
There was a problem hiding this comment.
LGTM. It'd be best not to touch random lines next PRs, you can probably set you IDE to only format changed lines.
Thanks for the review @ShivaChen
…f.condition, scf.while (llvm#636) This pr intruduces CIRConditionLowering and CIRWhileLowering for lowering to scf.
…f.condition, scf.while (llvm#636) This pr intruduces CIRConditionLowering and CIRWhileLowering for lowering to scf.
…f.condition, scf.while (llvm#636) This pr intruduces CIRConditionLowering and CIRWhileLowering for lowering to scf.
…f.condition, scf.while (llvm#636) This pr intruduces CIRConditionLowering and CIRWhileLowering for lowering to scf.
…f.condition, scf.while (#636) This pr intruduces CIRConditionLowering and CIRWhileLowering for lowering to scf.
This pr intruduces CIRConditionLowering and CIRWhileLowering for lowering to scf.