This is an archive of the discontinued LLVM Phabricator instance.

[InstCombine] Combine const GEP chains
ClosedPublic

Authored by dtcxzyw on Apr 26 2023, 12:07 AM.

Details

Summary

This patch reverts rGae739aefd7473517d3f08b5c8d08a66c7f469198 to address performance regressions reported by our CI after rG2ec1d0f427c7822540352c0c14d057e7bfe4f77b.

For example:

define ptr @const_gep_chain(ptr %p, i64 %a) {
    %p1 = getelementptr inbounds i8, ptr %p, i64 %a
    %p2 = getelementptr inbounds i8, ptr %p1, i64 1
    %p3 = getelementptr inbounds i8, ptr %p2, i64 2
    %p4 = getelementptr inbounds i8, ptr %p3, i64 3
    ret ptr %p4
}

The last three GEPs will not be folded since rG2ec1d0f427c7822540352c0c14d057e7bfe4f77b.

I think it is appropriate to remove this code because there is no compile-time regression reported in our benchmarks.

Diff Detail

Event Timeline

dtcxzyw created this revision.Apr 26 2023, 12:07 AM
Herald added a project: Restricted Project. · View Herald TranscriptApr 26 2023, 12:07 AM
Herald added a subscriber: hiraditya. · View Herald Transcript
dtcxzyw requested review of this revision.Apr 26 2023, 12:07 AM
nikic accepted this revision.Apr 26 2023, 12:17 AM

LGTM. If we need to bring this check back, it will need a stronger condition.

This revision is now accepted and ready to land.Apr 26 2023, 12:17 AM
dtcxzyw updated this revision to Diff 517215.Apr 26 2023, 9:54 AM

Update tests

nikic added inline comments.Apr 26 2023, 9:58 AM
llvm/test/Transforms/LoopVectorize/AArch64/vector-reverse-mask4.ll
1

What's going on with the test changes in this file? I don't really get how this patch could cause them.

dtcxzyw updated this revision to Diff 517454.Apr 26 2023, 11:32 PM

Update tests

dtcxzyw added inline comments.Apr 26 2023, 11:34 PM
llvm/test/Transforms/LoopVectorize/AArch64/vector-reverse-mask4.ll
1

Sorry, I just forgot to build the AArch64 target.

dtcxzyw marked an inline comment as done.Apr 26 2023, 11:35 PM
nikic added inline comments.Apr 27 2023, 12:08 AM
llvm/test/Transforms/LoopVectorize/consecutive-ptr-uniforms.ll
454

Would it be enough to only update these two lines in this test, rather than delete the whole block?

dtcxzyw updated this revision to Diff 517622.Apr 27 2023, 10:04 AM

Update tests

dtcxzyw marked an inline comment as done.Apr 27 2023, 10:05 AM
nikic accepted this revision.Apr 27 2023, 12:23 PM

LGTM

nikic added a comment.Apr 30 2023, 7:43 AM

Do you have commit access? If not, can you let me know the Name <email> to use for the commit?

dtcxzyw updated this revision to Diff 518459.May 1 2023, 8:57 AM

Resolve conflicts before committing

This revision was landed with ongoing or failed builds.May 1 2023, 9:29 AM
This revision was automatically updated to reflect the committed changes.