-
Notifications
You must be signed in to change notification settings - Fork 404
[Arcilator] Print full values rather than truncating them. #9120
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
Conversation
|
Surprisingly this fails on windows, despite:
Will debug this more in detail if this is useful to merge |
fzi-hielscher
left a comment
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! I've also had the urge to change this on several occasions.
test/arcilator/emit-values.mlir
Outdated
| %v3 = arith.constant 0x0123456789ABCDEF : i64 | ||
| arc.sim.emit "v3", %v3 : i64 | ||
|
|
||
| // CHECK: v4 = 0000000000001 |
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.
The zero padding behavior is a bit weird. Here we've got 13 characters for a i64. We should consistently either trim all leading zeros or pad to the full width of the type. I'd prefer the latter option.
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.
Makes sense. I was trying not to modify the default behaviour. Updated now to always show all leading zeros.
|
Thanks! |
6a559c8 to
f125f7f
Compare
|
I see that by printing leading 0, some of the integration tests fail. Please advice what's the best solution. We could also use a custom printf function that trims all leading zeros correctly (also for >64bit values) which we link into the jit. |
fzi-hielscher
left a comment
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.
We'll definitely have to add custom print/formatting functions as part of a runtime library in the future. I had a go at that last year and learned the hard way that, to make it work consistently across platforms, there are a bunch of pitfalls regarding ABIs, libc implementations and linkage. It's doable, but not as straightforward as one would hope.
Until then simply adapting the expected output of the integration tests to the new format should be fine. You could use a regex to ignore leading zeroes or just explicitly write them in.
| // Cast the value to a size_t. | ||
| // FIXME: like the rest of MLIR, this assumes sizeof(intptr_t) == | ||
| // sizeof(size_t) on the target architecture. | ||
| Value toPrint = adaptor.getValue(); | ||
| SmallVector<Value> printfVariadicArgs; | ||
| DataLayout layout = DataLayout::closest(op); | ||
| llvm::TypeSize sizeOfSizeT = | ||
| layout.getTypeSizeInBits(rewriter.getIndexType()); | ||
| assert(!sizeOfSizeT.isScalable() && | ||
| sizeOfSizeT.getFixedValue() <= std::numeric_limits<unsigned>::max()); |
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.
Do we still need sizeOfSizeT?
|
Updated to add either a single leading zero or a 0* regex to the integration tests. I removed the size_t part too and changed slightly the implementation to distinguish between first and other chunks rather than only remainingBits. OOC, regarding your point on ABI compatible prints: Wouldn't it be possible to create a vararg custom print that takes in int64 values, skips all leading ones that are zero - and starts printing from the first non-null value and rest? Probably not the most efficient but wouldn't that work irrespective of ABI ? |
fzi-hielscher
left a comment
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.
Great, thanks!
OOC, regarding your point on ABI compatible prints: Wouldn't it be possible to create a vararg custom print that takes in int64 values, skips all leading ones that are zero - and starts printing from the first non-null value and rest? Probably not the most efficient but wouldn't that work irrespective of ABI ?
Yeah, that should work. I'd still be hesitant to use a variadic function. The arcane ABI rules make it hard to interface with anything that isn't C, e.g. rust's c_variadic is still unstable. I think the best way to deal with this, which should allow us to use types beyond simple integers, is to allocate a "marshaling buffer", manually arrange the raw argument data within it and then pass a pointer and some sort of type descriptor to the external function.
test/Dialect/Arc/lower-sim.mlir
Outdated
| // CHECK: %[[lower:.*]] = llvm.trunc %[[arg0]] | ||
| // CHECK: %[[shifted:.*]] = llvm.lshr %[[arg0]], %[[shift]] | ||
| // CHECK: %[[higher:.*]] = llvm.trunc %[[shifted]] | ||
| // %CHECK: llvm.call @printf(%[[format_str3_ptr]], %[[higher]], %[[lower]]) |
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.
| // %CHECK: llvm.call @printf(%[[format_str3_ptr]], %[[higher]], %[[lower]]) | |
| // CHECK: llvm.call @printf(%[[format_str3_ptr]], %[[higher]], %[[lower]]) |
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, fixed
Implemented by slicing the value into size_t chunks and passing them as variadic args to printf.
Implemented by slicing the value into size_t chunks and passing them as variadic args to printf.
Example: