[PATCH] D24557: [Loop Vectorizer] Simplified GEP cloning. NFC.

[Michael Kuperstein via llvm-commits]

mkuper added a comment.

In https://reviews.llvm.org/D24557#542905, @delena wrote:

> > assert(i == getGEPInductionOperand(Gep) || Legal->isUniform(Gep->getOperand(i) || Legal->isInductionVariable(Gep->getOperand(i)))
>
>
> I'm not sure that this is the right check. My problem in https://reviews.llvm.org/D20789 was in these assertions that started to fail after more deep GEP analysis.
>  Re-checking GEP means checking correctness of isConsecutivePtr(Ptr).
>  In the new version (20789) isConsecutivePtr() uses PSE, which includes versioning and picks up more GEPs than we do now. 
>  And we can just rely on correctness of this function. 
>  GEP cloning does not depend on index role inside the loop and getScalarValue() covers all of them.


I want https://reviews.llvm.org/D20789 to go back in (there's a real performance issue I have that's affected by it :) ), but this scares me.
In theory, the assert I wrote above (or something similar) should not be failing. If it's failing, we are either missing a case, or there's a real subtle bug somewhere. I'd really like to know which one it is. We can't just say "we don't understand why this assert is failing, so let's not have it there". Once we do understand it, and it turns out that, say, the real assertion is very complex and requires replicating the logic of isConsecutivePtr() - I'll be perfectly OK with not adding it.

To quote G.K. Chesterton:

> In the matter of reforming things, as distinct from deforming them, there is one plain and simple principle; a principle which will probably be called a paradox. There exists in such a case a certain institution or law; let us say, for the sake of simplicity, a fence or gate erected across a road. The more modern type of reformer goes gaily up to it and says, “I don’t see the use of this; let us clear it away.” To which the more intelligent type of reformer will do well to answer: “If you don’t see the use of it, I certainly won’t let you clear it away. Go away and think. Then, when you can come back and tell me that you do see the use of it, I may allow you to destroy it.



Repository:
  rL LLVM

https://reviews.llvm.org/D24557