-
Notifications
You must be signed in to change notification settings - Fork 317
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
Enable querying constants and value kinds #936
Conversation
This PR also fixes #727 |
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.
Thanks! This is a useful feature. Please add unit tests.
ffi/value.cpp
Outdated
@@ -309,6 +309,34 @@ LLVMPY_DisposeOperandsIter(LLVMOperandsIteratorRef GI) { | |||
delete llvm::unwrap(GI); | |||
} | |||
|
|||
API_EXPORT(LLVMBool) |
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.
Please use bool
rather than LLVMBool
since that's that's the type preferred by the Python API, even though they should be the same.
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.
Fixed, thank you!
@apmasell Removed LLVMBool from Python-facing API, added tests, and fixed formatting. |
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.
Looks good. Thank you!
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.
I have some additional concerns mainly the lack of docs for the new code, and the lack of test coverage for some cases.
llvmlite/binding/value.py
Outdated
value = ffi.lib.LLVMPY_GetConstantFPValue(self, | ||
byref(accuracy_loss)) | ||
if accuracy_loss.value: | ||
warnings.warn( |
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.
Needs to add test for the warning
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.
@sklam For some reason, despite how much I try, I can't seem to trigger the losesInfo
boolean with a single constant (maybe this appears during passes). Not sure how to proceed here, any suggestions?
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.
Sorry for the delay.
After some digging into the LLVM source, the floating-point constants are encoded as double-precision so there will not be any information loss here. To force info loss, we will need to use bigger types like the long-double. For example: %const = fadd fp128 0xLF3CB1CCF26FBC178452FB4EC7F91DEAD, 0xLF3CB1CCF26FBC178452FB4EC7F91973F
. This will require converting the 128-bit float to 64-bit float thus triggering the warning.
However, this brings to my attention that a warning is not appropriate here. This API is not giving user a choice to read float bigger than double-precision. In the LLVM OCAML binding, the similar function will return None
(https://github.com/llvm/llvm-project/blob/1defa781243f9d0bc66719465e4de33e9fb7a243/llvm/bindings/ocaml/llvm/llvm_ocaml.c#L1030-L1032). I think the Python API should raise an exception.
Also, maybe it is better to have separate functions for int constant and float constant, so that the float version can provide more control on what to do if information is lost. For instance, if user is okay with the info loss, they can force the downcast.
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.
That's great, I agree and already modified the constant float test. Only thing I'm not sure about is the separate functions for int and float constants. One of the main benefits of Python is that you can return multiple types. Maybe this should be another keyword argument, and keyword args should act like preferences?
I am already extending the behavior to support other types of constants (e.g. structs), and it was beneficial to be able to call the same method on the nested data: tbennun@3992ea1#diff-27ab06b9fa73e854c37e5ccf261269588cd8e02fadbd71c788102bc46c4a2f1bR390
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.
Maybe this should be another keyword argument, and keyword args should act like preferences?
Yes, that works too
I am already extending the behavior to support other types of constants (e.g. structs), and it was beneficial to be able to call the same method on the nested data: tbennun@3992ea1#diff-27ab06b9fa73e854c37e5ccf261269588cd8e02fadbd71c788102bc46c4a2f1bR390
👍
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.
@sklam all changes are complete and the PR is ready for re(re)review.
@sklam Added documentation and more tests (apart from testing the warning, which I couldn't reproduce but is part of the LLVM interface, see above comment). I also noticed that adding one argument to |
Any updates on this @sklam ? This is not waiting for me as of two months ago. |
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.
Thanks for adding to the docs. Those looks great.
However, I have followed the rabbit hole and discovered more details to be considered for get_constant_value
and the info loss issue. See below.
llvmlite/binding/value.py
Outdated
value = ffi.lib.LLVMPY_GetConstantFPValue(self, | ||
byref(accuracy_loss)) | ||
if accuracy_loss.value: | ||
warnings.warn( |
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.
Sorry for the delay.
After some digging into the LLVM source, the floating-point constants are encoded as double-precision so there will not be any information loss here. To force info loss, we will need to use bigger types like the long-double. For example: %const = fadd fp128 0xLF3CB1CCF26FBC178452FB4EC7F91DEAD, 0xLF3CB1CCF26FBC178452FB4EC7F91973F
. This will require converting the 128-bit float to 64-bit float thus triggering the warning.
However, this brings to my attention that a warning is not appropriate here. This API is not giving user a choice to read float bigger than double-precision. In the LLVM OCAML binding, the similar function will return None
(https://github.com/llvm/llvm-project/blob/1defa781243f9d0bc66719465e4de33e9fb7a243/llvm/bindings/ocaml/llvm/llvm_ocaml.c#L1030-L1032). I think the Python API should raise an exception.
Also, maybe it is better to have separate functions for int constant and float constant, so that the float version can provide more control on what to do if information is lost. For instance, if user is okay with the info loss, they can force the downcast.
This PR adds bindings for obtaining whether a value is constant, what its
ValueKind
is, and its exact constant value in certain cases (integer, floating point; in other cases the IR string is returned instead).This is useful for inspecting constant values in Python code.