Page MenuHomePhabricator

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

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

Details

Summary

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

Please clang-format the function

6108

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
6104

oops. copy paste issue when resolve the conflict.

6108

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
6111

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
6111

Or maybe std::optional?

Address comments.

RKSimon added inline comments.Apr 30 2019, 4:03 AM
llvm/lib/CodeGen/SelectionDAG/DAGCombiner.cpp
6245–6248

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

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.