This is an archive of the discontinued LLVM Phabricator instance.

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

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

Diff Detail

Repository
rL LLVM

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
llvm/lib/CodeGen/SelectionDAG/DAGCombiner.cpp
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.
llvm/lib/CodeGen/SelectionDAG/DAGCombiner.cpp
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
llvm/lib/CodeGen/SelectionDAG/DAGCombiner.cpp
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
llvm/lib/CodeGen/SelectionDAG/DAGCombiner.cpp
6135 ↗(On Diff #197050)

Or maybe std::optional?

Address comments.

RKSimon added inline comments.Apr 30 2019, 4:03 AM
llvm/lib/CodeGen/SelectionDAG/DAGCombiner.cpp
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

llvm/lib/CodeGen/SelectionDAG/DAGCombiner.cpp
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.