[CIR][ThroughMLIR] Support lowering cir.condition and cir.while to scf.condition, scf.while

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 cir.condition and cir.while to scf.condition, scf.while #636

Closed GaoXiangYa closed 3 months ago

GaoXiangYa commented 4 months ago

This pr intruduces CIRConditionLowering and CIRWhileLowering for lowering to scf.

bcardosolopes commented 4 months ago

@ShivaChen can you review this and let us know if anything should be done regarding the loop canonicalization pass?

ShivaChen commented 4 months ago

@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.

GaoXiangYa commented 4 months ago

Hello, if you have review the code, and agree with me, I want to try to move the CIRWhileOpLowering to LowerCIRLoopToSCF.cpp file. @ShivaChen

ShivaChen commented 4 months ago

Hello, if you have review the code, and agree with me, I want to try to move the CIRWhileOpLowering to LowerCIRLoopToSCF.cpp file. @ShivaChen

Yes, I agree LowerCIRLoopToSCF.cpp could be right place to go. Thanks!

ShivaChen commented 4 months ago

@ShivaChen can you review this and let us know if anything should be done regarding the loop canonicalization pass?

It seems cir.while to scf.while have one to one mapping. cir.while.cond -> scf.while.before, cir.while.body -> scf.while.after cir.condition -> scf.condition So I assume the canonicalization might not require.

GaoXiangYa commented 4 months ago

Do you mean I add a SCFWhileLoop class ? And I will delete the header file. @ShivaChen

ShivaChen commented 4 months ago

Do you mean I add a SCFWhileLoop class ? And I will delete the header file. @ShivaChen

I think the way you implement CIRWhileOpLowering in https://github.com/llvm/clangir/pull/636/commits/e882b20609a492a9b62dc0da04a6289a952281cb make sense to me. We could introduce new class if we think there are more analysis need to be done in the future.

ShivaChen commented 3 months ago

Could you remove the changes of deleting empty lines and include headers in LowerCIRToMLIR.cpp?

GaoXiangYa commented 3 months ago

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

ShivaChen commented 3 months ago

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. Overall it's LGTM. Please wait for @bcardosolopes for another look. Thanks!