Page MenuHomePhabricator

[LLDB] Add basic floating point ops to IR interpreter
ClosedPublic

Authored by kpdev42 on May 25 2022, 1:39 AM.

Details

Summary

Patch adds support for fadd, fsub, fdiv, fmul and fcmp to IR interpreter.

~~~

OS Laboratory. Huawei RRI. Saint-Petersburg

Diff Detail

Event Timeline

kpdev42 created this revision.May 25 2022, 1:39 AM
Herald added a project: Restricted Project. · View Herald TranscriptMay 25 2022, 1:39 AM
kpdev42 requested review of this revision.May 25 2022, 1:39 AM
granata.enrico resigned from this revision.May 25 2022, 1:45 AM
DavidSpickett added a subscriber: DavidSpickett.

I know very little about the interpreter so someone else can comment there.

Certainly seems like a useful change. Added some generic comments.

lldb/test/API/lang/c/fpeval/TestFPEval.py
2

Update this comment

22

I'm pretty sure anything starting with test will do, so I'd just call this def test(self):.

23

Update this comment

52

Probably a silly question but do you need != here? I guess it becomes the inverse of == but wouldn't cost much to test it.

lldb/test/API/lang/c/fpeval/main.c
4

I don't think you need any of these includes, certainly the latter 2.

10

Small thing but could this line just be return 0; just saves a few cycles where you might think this line is being tested somehow when it really isn't.

kpdev42 updated this revision to Diff 440109.Jun 26 2022, 9:50 PM

Address review notes

Will the floating point emulation be perfect compared to actually JITing the code? If we are going to enable this feature I would like to see tests that actually JIT code do the same operations somehow to verify that things match up with the IR interpreted results produce. The main reason we didn't include floating point code in the IR interpreter before is it is harder to make things match up with exactly how things are going to be run if it were JIT'ed into the process. On x86_64 or x64 there are floating point settings that can be changed and affect how operations happen in the current thread. The main example I always think of is how older debuggers would create recursive descent parsers and evaluate floating point operations using native floating point types. I believe that our APFloat classes can do everything correctly, but I would like to see tests that verify that we get the same results as if we were running actual code in the process. For this you can add an extra function to the lldb/test/API/lang/c/fpeval/main.c source like:

typedef enum FloatOperation { add, subtract, multiply, ... };
double eval(double a, double b, FloatOperation op) {
  switch (op) {
  case add: return a+b;
  case subtract: return a-b;
  ...
  }
}
lldb/test/API/lang/c/fpeval/TestFPEval.py
33

You might be able to get away with not actually even creating a target or running to main if you define variables in your expression:

self.expect('expr --allow-jit false  -- float x=2.2; float y=4.4; x+y', ...
lldb/test/API/lang/c/fpeval/main.c
2–11

I don't think this file is even needed if you define variables locally in your own expression each time like I mentioned above ("expr float x=2.2; float y=4.4; x+y")

4–5

We should be testing "float" and "long double" as well to verify those work.

clayborg added inline comments.Jun 27 2022, 10:12 AM
lldb/test/API/lang/c/fpeval/TestFPEval.py
33–34

if we want to verify if the above "a+b" works as expected compared to JITed code, you can also run an expression like:

expr eval(a, b, add)

Then then we would want to compare the expression results to make sure the binary answer matches exactly. To do this, we will want to use the LLDB native APIs:

no_jit_options = lldb.SBExpressionOptions()
no_jit_options = expt_options.SetAllowJIT(False)
jit_options = lldb.SBExpressionOptions()
jit_options = expt_options.SetAllowJIT(True)
no_jit_value = frame.EvaluateExpression("a+b", no_jit_options)
jit_value = frame.EvaluateExpression("eval(a, b, add)", jit_options)
no_jit_data = no_jit_value.GetData()
jit_data = no_jit_value.GetData()

Then we need to compare the data byte for byte.

kpdev42 added inline comments.Jul 26 2022, 5:58 AM
lldb/test/API/lang/c/fpeval/TestFPEval.py
33

Actually I wanted the test to read floating point values from process memory, checking if they're passed correctly to IR along the way. This might be redundant, however your expression still needs a target to be executed.

33–34

IMO this will not work, because lldb always tries interpreter first and jitter is used only if interpreter fails. This may be circumvented by generating expression which interpreter cannot handle (function call?), however I don't see a point in doing it. Why not just check result value?

lldb/test/API/lang/c/fpeval/main.c
4–5

Long double is not supported by this patch, but can be added. However long double is platform dependent type, so it makes no sense at all comparing JIT and interpreted results

clayborg added inline comments.Jul 26 2022, 9:54 AM
lldb/test/API/lang/c/fpeval/TestFPEval.py
33–34

If you look at how I did the expression, the one that runs with "jit_options" calls a function to get the result. When a function is called, it causes the code to not be interpreted since the interpreter doesn't handle function calls., so this would work.

however I don't see a point in doing it. Why not just check result value?

My main concern with emulating the floating point stuff is getting a different value from host based interpretation than we would if we actually ran it on the actual hardware by jitting it.

labath added a subscriber: labath.Jul 27 2022, 6:54 AM
labath added inline comments.
lldb/test/API/lang/c/fpeval/main.c
4–5

I think that was Greg's point. Interpreted and JITted results should provide the same results even for platform dependent types. llvm's APFloat should know enough about the individual types in order to emulate the operations correctly, and lldb should know enough about the target in order to create the appropriate APFloat instance.

clayborg added inline comments.Jul 28 2022, 10:36 AM
lldb/test/API/lang/c/fpeval/main.c
4–5

Indeed. and if we are going to replace the float/double/long double interpretation, APFloat can handle that all as we must get the types from the ASTContext for the triple that has the right types anyway so this should be easy to make work.

Matt added a subscriber: Matt.Aug 2 2022, 10:26 PM
kpdev42 updated this revision to Diff 450244.Aug 5 2022, 12:15 AM

Update test case so it compares JIT’ed and interpreted FP division results and also check operations on float type. Patch doesn’t implement long double, because IR interpreter currently doesn’t support instruction argument size of more than 64 bit which includes both fp128 and int128. This deserves a separate patch, I think

clayborg accepted this revision.Aug 8 2022, 3:51 PM
This revision is now accepted and ready to land.Aug 8 2022, 3:51 PM
This revision was automatically updated to reflect the committed changes.