search

weld-project / weld

High-performance runtime for data analytics applications
https://www.weld.rs
BSD 3-Clause "New" or "Revised" License
2.99k stars 256 forks source link

Use ::libc::c_char as the character type #484

Closed filabrazilska closed 5 years ago

filabrazilska commented 5 years ago

On AArch64 the C char type is u8 not i8, resulting in compilation errors such as:

error[E0308]: mismatched types --> weld/src/codegen/llvm2/mod.rs:89:9 89 concat!($s, "\0").as_ptr() as *const i8 ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ expected u8, found i8 ... 930 c_str!(""), ---------- in this macro invocation 
= note: expected type `*const u8`
           found type `*const i8`
radujica commented 5 years ago

This seems like it can have much greater implications; do tests still pass with such change? What about the Python wrapper?

sppalkia commented 5 years ago

This seems okay since this macro is just used to pass arguments into the LLVM functions (it doesn't change types in the ABI), and the argument to those functions uses ibc::c_char anyways -- thanks for the patch!

filabrazilska commented 5 years ago

The concern about passing tests is valid -- the tests keep passing on intel but fail on missing hash implementation on arm (weld/src/codegen/llvm2/hash.rs:122). I'll look into how the hashing works later.

sppalkia commented 5 years ago

Hashing currently relies on a CRC32 intrinsic found in most (all?) recent Intel processors -- if those intrinsics aren't found on the machine, there is currently an unimplemented error that is thrown. It would be great to add something here! Hashing is implemented by providing a hash function from various integer widths to either a 32- or 64-bit integer, as defined here: https://github.com/weld-project/weld/blob/83c0b7a783657d5408b5eb4ed8c5938126f1ecc6/weld/src/codegen/llvm2/hash.rs#L144. To support ARM, we'd need to add an implementation of these functions that don't use the SSE 4.2 intrinsics.

On Mon, Sep 30, 2019 at 12:41 AM Filip Andres notifications@github.com wrote:

The concern about passing tests is valid -- the tests keep passing on intel but fail on missing hash implementation on arm (weld/src/codegen/llvm2/hash.rs:122). I'll look into how the hashing works later.

— You are receiving this because you modified the open/close state. Reply to this email directly, view it on GitHub https://github.com/weld-project/weld/pull/484?email_source=notifications&email_token=AAKMEY3EUKO3JUDYBJFBX2DQMD4WFA5CNFSM4I3KOIJ2YY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOD734MEA#issuecomment-536331792, or mute the thread https://github.com/notifications/unsubscribe-auth/AAKMEY3POQ3YZIWNXNRVAZ3QMD4WFANCNFSM4I3KOIJQ .

-- Shoumik