Skip to content
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

[BPF] lowering target address leaf nodes tconstpool #73667

Merged
merged 1 commit into from
Mar 6, 2024

Conversation

inclyc
Copy link
Member

@inclyc inclyc commented Nov 28, 2023

Adds custom lowering for these leaf nodes.

Please ref: #73668 for test coverage

@tamird
Copy link
Member

tamird commented Nov 28, 2023

Please add tests.

@inclyc
Copy link
Member Author

inclyc commented Nov 28, 2023

Please add tests.

These nodes are basic symbols and I don't know how to test it directly, however #73668 referenced tconstpool, would you like to see other nodes get test-coveraged in this way?

(I checked commits in other targets and they seem to commit these pattern without tests, e.g. bec7b16)

@inclyc inclyc marked this pull request as draft November 29, 2023 05:58
@inclyc inclyc force-pushed the users/inclyc/bpf-target-addr branch from c41b11a to 1839091 Compare December 10, 2023 06:51
@inclyc inclyc changed the title [BPF] lowering target address leaf nodes (tconstpool, tblockaddr, tjumptable) [BPF] lowering target address leaf nodes tconstpool Dec 10, 2023
@inclyc inclyc marked this pull request as ready for review December 10, 2023 07:08
@eddyz87
Copy link
Contributor

eddyz87 commented Jan 31, 2024

The code lgtm.
Constants pool usage matches other backends (e.g. x86) and #73668 triggers code paths from this MR.
It seems that there is no straightforward way to test this commit: all uses of getConstantPool/getTargetConstantPool I checked are internal to DAG translation of specific constructs.
It might be possible to make a unit test that constructs DAG and lowers it, but I think that's an overkill.

@yonghong-song , wdyt?

@inclyc
Copy link
Member Author

inclyc commented Mar 4, 2024

Kindly ping @yonghong-song 😃

This PR has been blocked for 1 month 😨 ! Could you please take a look?

@inclyc
Copy link
Member Author

inclyc commented Mar 6, 2024

I'd like to merge this now :)

Please comment here if there are any issue caused by this PR, and I would happy to revert it!

@inclyc inclyc merged commit cf922e5 into main Mar 6, 2024
6 checks passed
@inclyc inclyc deleted the users/inclyc/bpf-target-addr branch March 6, 2024 11:47
inclyc added a commit that referenced this pull request Apr 1, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants