Skip to content

[CodeGen][LLVM] Lower if_then_else() with select#11795

Closed
wrongtest-intellif wants to merge 1 commit intoapache:mainfrom
wrongtest-intellif:change_llvm_if_then_else_op
Closed

[CodeGen][LLVM] Lower if_then_else() with select#11795
wrongtest-intellif wants to merge 1 commit intoapache:mainfrom
wrongtest-intellif:change_llvm_if_then_else_op

Conversation

@wrongtest-intellif
Copy link
Contributor

@wrongtest-intellif wrongtest-intellif commented Jun 21, 2022

Hi, there. It seems better to use select directly for if_then_else() expr. Since if_then_else() is common in padding representation, it could hopefully save lots of cross-bb overheads.

My two remaining questions:
(1) Are there any places to test certain codegen features?
(2) Are there known llvm-based target which do not handle select instruction well?

@junrushao
Copy link
Member

Quick question: in TIR, T.Select means both branches are executed no matter which value the conditional is evaluated to - which brings benefits that make it vectorizable in certain cases. However, if the conditional is a safeguard to avoid access out of buffer boundary, replacing it with T.Select will lead the program to crash. Therefore, I would love to learn if our implementation here in LLVM codegen will lead to the same effect? Thanks a lot!

@wrongtest-intellif
Copy link
Contributor Author

wrongtest-intellif commented Jun 21, 2022

the conditional is a safeguard

Thanks for the reply! So is the if_then_else also take short-circuit evaluation semantic like if stmt? If so I think my replacement is unsafe. What about add a check like SideEffect(e) < kReadState?

@wrongtest-intellif
Copy link
Contributor Author

After some thinking, it seems to be better to make TIR transformation if_then_else() -> T.select if neccesary, thus do not complex the llvm codegen.

Thank you all the same! @junrushao1994

@junrushao
Copy link
Member

@wrongtest Thank you so much for the discussion!

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.

2 participants