This is an archive of the discontinued LLVM Phabricator instance.

reenable gep merging in some constrainted cases
Needs ReviewPublic

Authored by wmi on May 19 2015, 2:49 PM.

Details

Reviewers
qcolombet
hfinkel
Summary

In https://llvm.org/bugs/show_bug.cgi?id=23580, we saw a testcase for which loop vectorizer cannot generate wide load because it cannot recognize the load to be inter-interation consecutive without gep merging. That testcase showed the usage of gep merging in analysis besides basicaa.

To make the consecutive load analysis more effective, but still control the negative side effect of gep merging, the patch allows the source gep to be merged with the target gep if 1. the source gep only has one use and 2. both geps are in the same BB. This should cover the most common cases.

spec2000 and internal performance testing show no regression.

Diff Detail

Repository
rL LLVM

Event Timeline

wmi updated this revision to Diff 26096.May 19 2015, 2:49 PM
wmi retitled this revision from to reenable gep merging in some constrainted cases.
wmi updated this object.
wmi edited the test plan for this revision. (Show Details)
wmi added reviewers: qcolombet, hfinkel.
wmi set the repository for this revision to rL LLVM.
wmi added subscribers: Unknown Object (MLST), davidxl.
qcolombet edited edge metadata.May 19 2015, 3:00 PM

Hi Wei,

Correct me if I am wrong, but this is a tuning of the heuristic from http://reviews.llvm.org/D8911, right?

Given that you actually see a regression now (and the fact that after discussing with Chandler, I am not convinced the initial change makes sense), I would suggest that you revert r235455 and that we start again the discussion on how to properly fix that.

Thanks,
-Quentin

wmi added a comment.May 19 2015, 3:10 PM

Hi Quentin,

Yes, it is a tuning of the heuristic from http://reviews.llvm.org/D8911.

Because the regression I saw here has much smaller negative impact
compared with the original negative impact of gep merging, is it
possible that we leave r235455 there while we discuss a better fix?
Because in this way we can check is there any other analysis affected
by disabling gep merging.

Thanks,
Wei.

wmi updated this revision to Diff 26295.May 21 2015, 5:49 PM
wmi edited edge metadata.

Hi Quentin,

I put the disabling GEP merging and related heuristic tuning under an internal option and make it the default for now. By the way, I think it is better that LoopVectorizationLegality::isConsecutivePtr can check more than one level gep, just like what DecomposeGEPExpression does now in Basicaa. I will try that in another patch.

Thanks,
Wei.

Your test cases are quite large, can you please make them a little smaller?

lib/Transforms/InstCombine/InstructionCombining.cpp
78–80

This looks a bit funny, can you clang-format this?

test/Transforms/InstCombine/gep-merge1.ll
2

Please run -instcombine instead of -O2.

wmi updated this revision to Diff 26307.May 21 2015, 10:46 PM

Your test cases are quite large, can you please make them a little smaller?

Done.

+static cl::opt<bool>
+ AggressiveGepMerging("aggr-gep-merging", cl::init(false), cl::Hidden,
+ cl::desc("Enable GEP merging for most cases."));

+

This looks a bit funny, can you clang-format this?

It has already been processed by clang-format.

Comment at: test/Transforms/InstCombine/gep-merge1.ll:2
@@ +1,3 @@
+; PR23580
+; RUN: opt < %s -O2 -S | FileCheck %s

+

Please run -instcombine instead of -O2.

I changed -O2 to necessary passes for the test.

Thanks,
Wei.