catalyst
catalyst copied to clipboard
Remove 'puts' error message from async lowering
Context: The async lowering will add a puts and a message about values in an error state. We should remove them as we now return an exception. Not really a bug, but a user experience issue.
Description of the Change: We renamed the RemoveAbortInsertCallTransform to RemoveAbortAndPutsInsertCallTransform to avoid creating a new transform with boiler-plate code. We just have added the methods and functions that allowed us to, first, collect the blocks including calls to puts, and second, to erase those call operations.
Benefits: avoid having to create a new transform.
Possible Drawbacks:
- Maybe we want to separate aborts logic from and puts logic?
- Maybe we can delete the puts call ops at the very moment we check for the blocks containing them?
Related GitHub Issues: https://github.com/PennyLaneAI/catalyst/issues/514
TODO:
- [ ] Add tests
- [ ] Modify changelog.md
[sc-59514]
Hello. You may have forgotten to update the changelog!
Please edit doc/changelog.md on your branch with:
- A one-to-two sentence description of the change. You may include a small working example for new features.
- A link back to this PR.
- Your name (or GitHub username) in the contributors section.
Hello. You may have forgotten to update the changelog!
Please edit doc/changelog.md on your branch with:
- A one-to-two sentence description of the change. You may include a small working example for new features.
- A link back to this PR.
- Your name (or GitHub username) in the contributors section.
Hi @rauletorresc , thanks for this work! I am not really experienced in this code area, so let me ask a few general questions to understand the situation better:
- How do these
puts(andaborts) appear in code? If we do not want them to exist, can we prevent their appearance rather than removing afterwards? - I see that
collectPutsBlocksscans users of llvm conditions currently. This seems a rather specific criteria to me. Is it what we want here? Can we think of other locations of puts?
How do these puts (and aborts) appear in code? If we do not want them to exist, can we prevent their appearance rather than removing afterwards?
Hey Sergei! I can answer this. This code comes from the async runtime passes and transformations. These passes lower the async dialect into LLVM. The puts and aborts come from here.
If we do not want them to exist, can we prevent their appearance rather than removing afterwards?
It is true! That is possible, but that would involve forking / patching LLVM. I am not against this, but it is not something we have been doing.
I see that collectPutsBlocks scans users of llvm conditions currently. This seems a rather specific criteria to me. Is it what we want here? Can we think of other locations of puts?
There were two options when starting this. Delete all puts, no matter in which context they appear. Or delete only these ones. LLVM passes only insert puts here. I think it makes sense to remove just these puts, because, hypothetically in the future we might want to add puts in our generated code. A pass that would delete all of them, would also delete these other ones. But I understand if this view is not shared by everyone.
One more note that I shared with @rauletorresc, this pass is failing likely because we modify too much and the fold passes between each transformation is folding away the (now likely empty) basic block where the aborts message used to be located.