This is an archive of the discontinued LLVM Phabricator instance.

[Scalar] Add support for 512-bits values.
ClosedPublic

Authored by davide on Jan 24 2019, 5:27 PM.

Details

Reviewers
clayborg
Summary

rdar://problem/46886288

Event Timeline

davide created this revision.Jan 24 2019, 5:27 PM

@clayborg, this is fairly mechanical, but I would appreciate if you can sign it off.

aprantl added inline comments.
lldb/source/Utility/RegisterValue.cpp
159

I'm curious, everything else in this patch refers to 512, how does this fit in?

lldb/source/Utility/Scalar.cpp
168

std::reverse perhaps?

davide marked 2 inline comments as done.Jan 25 2019, 12:58 PM
davide added inline comments.
lldb/source/Utility/RegisterValue.cpp
159

512 bits / 8 (bits per byte) = 64 bytes.
I can change the number to be in bits, if you want.

lldb/source/Utility/Scalar.cpp
168

We might want to change this everywhere.

clayborg accepted this revision.Jan 28 2019, 8:33 AM
This revision is now accepted and ready to land.Jan 28 2019, 8:33 AM

Thanks for the review, Greg!

zturner added inline comments.
lldb/source/Utility/Scalar.cpp
161

I'm confused. You say it returns a pointer to an array of four uint64_t values, but here you're clearly swapping the order of eight uint64_t values. Is the comment wrong?

Anyway, how about:

std::swap(apint_words[0], apint_words[7]);
std::swap(apint_words[1], apint_words[6]);
std::swap(apint_words[2], apint_words[5]);
std::swap(apint_words[3], apint_words[4]);
davide marked an inline comment as done.Jan 28 2019, 10:30 AM
davide added inline comments.
lldb/source/Utility/Scalar.cpp
161

Yes the comment is wrong. I need to update it.

davide closed this revision.Jan 30 2019, 10:05 AM
davide marked an inline comment as done.Jan 30 2019, 10:23 AM
davide added inline comments.
lldb/source/Utility/Scalar.cpp
168

I updated the comment but I can't really easily use std::swap or std::reverse because they operate on vectors and the unit of currency here is an array.
I thought about initializing a vector from the array we read and then call std::reverse (and then covert back to a pointer), but that didn't seem better than what we have now (or, at least, less ugly).

zturner added inline comments.Jan 30 2019, 10:40 AM
lldb/source/Utility/Scalar.cpp
168

I'm confused why std::swap(swapped_words[7], swapped_words[0]); and similar for the other 4 doesn't work though.

davide marked an inline comment as done.Jan 30 2019, 10:49 AM
davide added inline comments.
lldb/source/Utility/Scalar.cpp
168
/Users/davide/llvm-monorepo/llvm-mono/llvm/tools/lldb/source/Utility/Scalar.cpp:144:7: error: no matching function for call to 'swap'
      std::swap(apint_words[0], apint_words[3]);
      ^~~~~~~~~
/Applications/Xcode4.app/Contents/Developer/Toolchains/XcodeDefault.xctoolchain/usr/include/c++/v1/type_traits:4515:1: note: candidate template ignored: requirement 'is_move_assignable<const unsigned long long>::value' was not satisfied [with _Tp = const unsigned long long]
swap(_Tp& __x, _Tp& __y) _NOEXCEPT_(is_nothrow_move_constructible<_Tp>::value &&