-
Notifications
You must be signed in to change notification settings - Fork 31
feat: extend LiteralDict lowering with runtime support (constant-path tests) #213
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
Changes from 10 commits
d2a49a0
32bafd7
1b141af
62e342c
da25c60
41fd52e
02da402
e8e0ecd
75894c1
89f75cc
60683bf
6093d2b
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -96,3 +96,32 @@ def test_literal_dict_heterogeneous_constants_unsupported( | |
| } | ||
| ) | ||
| ) | ||
|
|
||
|
|
||
| @pytest.mark.parametrize("builder_class", [LLVMLiteIR]) | ||
| def test_literal_dict_const_lowering( | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. i have changed the test name as test never enters runtime lowering.Because
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. You're right that this path always hits the constant fast-path since LiteralInt32 lowers to ir.Constant. I updated the test name to test_literal_dict_const_lowering.
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. You should ideally test a non-constant key/value. uh for example
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Thanks for the suggestions, @yuvimittal Currently the tests run outside a function context, so runtime lowering cannot safely emit IR (no builder block). Because of that, the implementation falls back to constant lowering in tests. I agree that ideally we should test a true non-constant case (e.g., using a variable like
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @m-akhil-reddy , but the PR title and description doesnt align with existing tests
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @yuvimittal , I'll update the PR title/description to clarify that the implementation includes the runtime lowering path, while the current tests cover the constant fast-path due to running outside a function context.
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I have updated the PR title and description to clarify that the implementation includes the runtime lowering path while the current tests cover the constant fast-path due to running outside a function context. |
||
| builder_class: Type[Builder], | ||
| ) -> None: | ||
| """ | ||
| title: Runtime LiteralDict lowering (non-constant path) | ||
| parameters: | ||
| builder_class: | ||
| type: Type[Builder] | ||
| """ | ||
| builder = builder_class() | ||
| visitor = cast(LLVMLiteIRVisitor, builder.translator) | ||
| visitor.result_stack.clear() | ||
|
|
||
| visitor.visit( | ||
| astx.LiteralDict( | ||
| elements={ | ||
| astx.LiteralInt32(1): astx.LiteralInt32(2), | ||
| } | ||
| ) | ||
| ) | ||
|
|
||
| result = visitor.result_stack.pop() | ||
|
|
||
| # When no function context exists, runtime lowering falls back to constant | ||
| assert isinstance(result, ir.Constant) | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. better test
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I added assert result.type.count == 1 so the test also verifies the dictionary layout. |
||
| assert isinstance(result.type, ir.ArrayType) | ||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LLVM actually expects target pointer-sized integers for GEP indices on some targets. so the safer approach would be
i32 = self._llvm.INT32_TYPEThere was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I replaced ir.IntType(32) with self._llvm.INT32_TYPE to follow the project's LLVM type definitions.
thanks.