This is an archive of the discontinued LLVM Phabricator instance.

Load Combine : Combine Loads formed from GEPS with negative indexes
ClosedPublic

Authored by rriddle on Jun 18 2016, 5:55 PM.

Diff Detail

Event Timeline

rriddle updated this revision to Diff 61181.Jun 18 2016, 5:55 PM
rriddle retitled this revision from to Load Combine : Combine Loads formed from GEPS with negative indexes .
rriddle updated this object.
rriddle added reviewers: Bigcheese, majnemer.
rriddle added a subscriber: llvm-commits.
majnemer requested changes to this revision.Jun 18 2016, 6:05 PM
majnemer edited edge metadata.

I would avoid using an in-band value as a sentinel, there can be unintended consequences. Consider what happens when the GEP index is 9223372036854775807.

Instead, I'd use a separate bool indicating that PrevOffset holds a good value or make PrevOffset an Optional.

/Users/rriddle/Desktop/llvm/llvm/lib/Transforms/Scalar/LoadCombine.cpp
143

While I know of no platform that LLVM supports where LLONG_MAX != INT64_MAX, we would still prefer INT64_MAX.

144

Hmm, any reason why PrevSize had to become an int64_t ?

/Users/rriddle/Desktop/llvm/llvm/test/Transforms/LoadCombine/load-combine-negativegep.ll
2

Please don't run instcombine, we try to minimize the number of passes involved in each test.

This revision now requires changes to proceed.Jun 18 2016, 6:05 PM
rriddle updated this revision to Diff 61183.Jun 18 2016, 6:15 PM
rriddle edited edge metadata.

Fixed the size type change mistake. Changed the iteration variable to be a separate value.

majnemer accepted this revision.Jun 18 2016, 6:20 PM
majnemer edited edge metadata.

LGTM

This revision is now accepted and ready to land.Jun 18 2016, 6:20 PM

Hi River,

Do you need someone to commit this for you?

rriddle added a comment.EditedJun 18 2016, 7:20 PM

Hey David, This is my first patch so Im not too sure about the process for committing. Accordingly, Im fairly certain that I don't have commit access.
Thanks River

This revision was automatically updated to reflect the committed changes.