Page MenuHomePhabricator

[NFC] Add a static function to do the endian check

Authored by steven.zhang on Apr 27 2019, 11:18 PM.



Add a new function to do the endian check, as I will commit another patch later, which will also need the endian check. The coming patch is trying to combine several stores, as what MatchLoadCombine did. The opportunity is found from spec2017.

Diff Detail


Event Timeline

steven.zhang created this revision.Apr 27 2019, 11:18 PM
Herald added a project: Restricted Project. · View Herald TranscriptApr 27 2019, 11:18 PM
Herald added a subscriber: hiraditya. · View Herald Transcript
RKSimon added inline comments.Apr 28 2019, 1:14 AM
6128 ↗(On Diff #197001)

Please clang-format the function

6132 ↗(On Diff #197001)

Anyway that we can avoid duplicate copies of LittleEndianByteAt + BigEndianByteAt?

steven.zhang marked 2 inline comments as done.Apr 28 2019, 4:14 AM
steven.zhang added inline comments.
6128 ↗(On Diff #197001)

oops. copy paste issue when resolve the conflict.

6132 ↗(On Diff #197001)

Maybe, remove the LittleEndianByteAt + BigEndianByteAt for load combine ? As it is a really simple helper function, and it is only used in one place now

auto MemoryByteOffset = [&] (ByteProvider P) {
    assert(P.isMemory() && "Must be a memory byte provider");
    unsigned LoadBitWidth = P.Load->getMemoryVT().getSizeInBits();
    assert(LoadBitWidth % 8 == 0 &&
           "can only analyze providers for individual bytes not bit");
    unsigned LoadByteWidth = LoadBitWidth / 8;
    return IsBigEndianTarget ? LoadByteWidth - P.ByteOffset - 1 : P.ByteOffset;
steven.zhang marked an inline comment as done.

Update the patch according to the reviewer comments.

apilipenko added inline comments.Apr 29 2019, 9:23 AM
6135 ↗(On Diff #197050)

Small stylistic suggestion. This function essentially returns a tri-state: big, little, neither. An explicit enum for the return value might be a cleaner way to express this.

nemanjai added inline comments.Apr 29 2019, 10:45 AM
6135 ↗(On Diff #197050)

Or maybe std::optional?

Address comments.

RKSimon added inline comments.Apr 30 2019, 4:03 AM
6245 ↗(On Diff #197260)

Maybe call this IsBigEndian?

Update local variable name.

RKSimon accepted this revision.May 5 2019, 5:23 AM

LGTM - with one minor

6246 ↗(On Diff #198166)

Maybe if (!IsBigEndian.hasValue()) ?

This revision is now accepted and ready to land.May 5 2019, 5:23 AM

ok. Use the hasValue instead of the implicit bool convert operator to make the code clear.

This revision was automatically updated to reflect the committed changes.