llvm / clangir

A new (MLIR based) high-level IR for clang.
https://clangir.org
Other
351 stars 95 forks source link

[CIR][ThroughMLIR] Support lowering ForOp to scf #605

Closed ShivaChen closed 4 months ago

ShivaChen commented 4 months ago

This commit introduces CIRForOpLowering for lowering to scf.

The initial commit only support increment loop with lt or le comparison.

bcardosolopes commented 4 months ago

Nice, thanks! How does this relate to the other loop canonicalization PR?

I'd be good if you can split the implementation into another file, so we can mitigate this early on, differently from the current state of LowerToLLVM.cpp.

ShivaChen commented 4 months ago

Nice, thanks! How does this relate to the other loop canonicalization PR?

WIth the other PR which guarantee IV in the LHS of loop comparison, we could use RHS to search the loop boundary and hoisting the boundary operations out of the loop would allow the conversion using the value as input operand for SCF loop.

I'd be good if you can split the implementation into another file, so we can mitigate this early on, differently from the current state of LowerToLLVM.cpp.

It seems to be a good idea. I will work in this direction. Thanks!

ShivaChen commented 4 months ago

I'd be good if you can split the implementation into another file, so we can mitigate this early on, differently from the current state of LowerToLLVM.cpp.

It seems to be a good idea. I will work in this direction. Thanks!

I tried to implement a pass before ConvertCIRToMLIRPass which only convert CIR for loop to SCF. Because SCF for loop required the inputs are builtin integer types(index or i32/i64), the operand checking will report error. It seems CIRForOpLowering might live in ConvertCIRToMLIRPass to make sure the operations before loops have been converted to MLIR. Would you like me to try other way to split the implementation or live CIRForOpLowering as it is?

bcardosolopes commented 4 months ago

I tried to implement a pass before ConvertCIRToMLIRPass which only convert CIR for loop to SCF. Because SCF for loop required the inputs are builtin integer types(index or i32/i64), the operand checking will report error. It seems CIRForOpLowering might live in ConvertCIRToMLIRPass to make sure the operations before loops have been converted to MLIR. Would you like me to try other way to split the implementation or live CIRForOpLowering as it is?

Sorry for the confusion, my suggestion is only about the mechanical part of splitting the implementation in more files, no need to change the overall approach of the PR.

ShivaChen commented 4 months ago

I tried to implement a pass before ConvertCIRToMLIRPass which only convert CIR for loop to SCF. Because SCF for loop required the inputs are builtin integer types(index or i32/i64), the operand checking will report error. It seems CIRForOpLowering might live in ConvertCIRToMLIRPass to make sure the operations before loops have been converted to MLIR. Would you like me to try other way to split the implementation or live CIRForOpLowering as it is?

Sorry for the confusion, my suggestion is only about the mechanical part of splitting the implementation in more files, no need to change the overall approach of the PR.

I will put the implementation in a separate file. Thanks for the clarification.

GaoXiangYa commented 4 months ago

Hellow, I completed the CIRConditionLowering and CIRWhileLowering, do we need to put these codes in the LoweringCIRLoopTO SCF? @ShivaChen @bcardosolopes

ShivaChen commented 4 months ago

Hellow, I completed the CIRConditionLowering and CIRWhileLowering, do we need to put these codes in the LoweringCIRLoopTO SCF? @ShivaChen @bcardosolopes

It would be great addition. Thank you!