This is an archive of the discontinued LLVM Phabricator instance.

[SCEV] Keep SCEVExpander insert points consistent.
ClosedPublic

Authored by gberry on May 26 2016, 3:01 PM.

Details

Summary

Make sure that the SCEVExpander Builder insert point and any
saved/restored insert points are kept consistent (i.e. their Instruction
and BasicBlock match) when moving instructions in SCEVExpander.

This fixes an issue triggered by
http://reviews.llvm.org/D18001 [LSR] Create fewer redundant instructions.

Diff Detail

Repository
rL LLVM

Event Timeline

gberry updated this revision to Diff 58700.May 26 2016, 3:01 PM
gberry retitled this revision from to [SCEV] Keep SCEVExpander insert points consistent..
gberry updated this object.
gberry added a reviewer: sanjoy.
sanjoy requested changes to this revision.May 29 2016, 5:16 PM
sanjoy edited edge metadata.
sanjoy added inline comments.
include/llvm/Analysis/ScalarEvolutionExpander.h
97 ↗(On Diff #58700)

Add a description here on why this assert shouldn't fail.

include/llvm/IR/IRBuilder.h
229 ↗(On Diff #58700)

This new public interface looks like the kind of thing that will be inadvertently get misused later. How about duplicating the functionality of InsertPointGuard in SCEVInsertPointGuard and adding the getter and setter on just that class?

lib/Analysis/ScalarEvolutionExpander.cpp
917 ↗(On Diff #58700)

Why not Builder.SetInsertPoint(&*NewInsertPt)?

This revision now requires changes to proceed.May 29 2016, 5:16 PM
gberry updated this revision to Diff 59090.May 31 2016, 10:42 AM
gberry edited edge metadata.

Update to address Sanjoy's comments.

Hi Sanjoy,

Thanks for the review. I've made the changes you suggested. I'm not entirely pleased with the duplicating of InsertPointGuard, though I also wasn't pleased with changing the InsertPointGuard interface.

sanjoy accepted this revision.May 31 2016, 4:38 PM
sanjoy edited edge metadata.

lgtm with nits, thanks!

I'm not entirely pleased with the duplicating of InsertPointGuard, though I also wasn't pleased with changing the InsertPointGuard interface.

Yes, but it seems to be the lesser of two evils. :)

include/llvm/Analysis/ScalarEvolutionExpander.h
121 ↗(On Diff #59090)

Minor: add newline between declarations.

123 ↗(On Diff #59090)

Assert that this is empty when destroying a SCEVExpander.

188 ↗(On Diff #59090)

This should be a private function.

This revision is now accepted and ready to land.May 31 2016, 4:38 PM
This revision was automatically updated to reflect the committed changes.
gberry added a comment.Jun 1 2016, 1:10 PM

Thanks Sanjoy.

llvm/trunk/include/llvm/Analysis/ScalarEvolutionExpander.h
367

FYI, I had to make this a private member function to make fixupInsertPoints private.