Skip to content

[Codegen] Fix if_then_else codegen#16242

Merged
jinhongyii merged 5 commits intoapache:mainfrom
spectrometerHBH:fix
Jan 4, 2024
Merged

[Codegen] Fix if_then_else codegen#16242
jinhongyii merged 5 commits intoapache:mainfrom
spectrometerHBH:fix

Conversation

@spectrometerHBH
Copy link
Copy Markdown
Contributor

@spectrometerHBH spectrometerHBH commented Dec 14, 2023

  • Evaluating op.if_then_else with a?b:c is potentially dangerous as it might evaluate expressions in branches without a guard of condition.

  • While loop condition expression might require several var bindings generated, so we correct the while loop codegen from while (cond) to while (True)

Comment thread src/runtime/ndarray.cc Outdated
@vinx13
Copy link
Copy Markdown
Member

vinx13 commented Dec 17, 2023

isn't short circuiting the defined behavior for ternary expression?

@spectrometerHBH
Copy link
Copy Markdown
Contributor Author

isn't short circuiting the defined behavior for ternary expression?

When generating codes for a?b:c, there might be bindings for b or c generated ahead. These binding codes are not protected by condition a.

@jinhongyii jinhongyii merged commit ae7d9db into apache:main Jan 4, 2024
junrushao added a commit to junrushao/tvm that referenced this pull request Jan 7, 2024
* fix

* lint

* fix while loop

* clean

* clean

---------

Co-authored-by: Junru Shao <junrushao1994@gmail.com>
@ysh329
Copy link
Copy Markdown
Contributor

ysh329 commented Feb 17, 2025

isn't short circuiting the defined behavior for ternary expression?

When generating codes for a?b:c, there might be bindings for b or c generated ahead. These binding codes are not protected by condition a.

Hi, bohan, I recently am supporting Ternary Operator(like cond?true_branch:false_branch) in our device. I don't understand this modification. Can u explain this with a case? Thanks a lot!

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.

5 participants