Skip to content

[SandboxIR] Implement SIToFPInst #101374

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 1 commit into from
Jul 31, 2024
Merged

[SandboxIR] Implement SIToFPInst #101374

merged 1 commit into from
Jul 31, 2024

Conversation

vporpo
Copy link
Contributor

@vporpo vporpo commented Jul 31, 2024

This patch implements sandboxir::SIToFPInst which mirrors llvm::SIToFPInst.

@@ -1378,6 +1380,27 @@ class CastInst : public Instruction {
#endif
};

class SIToFPInst final : public CastInst {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I wonder if it's possible to do some CRTP magic to stamp out all of these instantiations much more succinctly. Let me mess around locally.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

see 1cc8adb

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah that works. Do you want to create a PR for this?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I could also try doing something similar for the tests, since there is a lot of repetition there too. The only downside is that if there is a failure the error reported won't be as easy to follow, but I doubt these tests will fail.

If we go this way, then the rest of the CastInst sub-classes can be a single PR which would be really nice.

Wdyt?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@vporpo vporpo merged commit 6d3317e into llvm:main Jul 31, 2024
5 of 7 checks passed
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