[PATCH] D35933: Eliminate TargetTransformInfo::isFoldableMemAccess()

[Jonas Paulsson via Phabricator via llvm-commits]

jonpa marked an inline comment as done.
jonpa added a comment.



In https://reviews.llvm.org/D35933#823866, @uweigand wrote:

> In https://reviews.llvm.org/D35933#823813, @jonpa wrote:
>
> > > I'm not sure about the common code changes -- what does simply calling isLegalAddressingMode instead of isFoldableMemAccessOffset do to targets that have not yet added support to do instruction-specific checks to the former?  I'd assume you'd see performance regressions there.
> > > 
> > > Maybe this transition should be done on a target-by-target basis.
> >
> > SystemZ was the only user of isFoldableMemAccessOffset() except for any out-of-tree targets.
>
>
> Well, even if they had no separate implementation, they would fall back to the default one (returning always true).   Now the code will do a call to isLegalAddressingMode instead.  Is is clear that this will either be no change (i.e. also return always true), or else an lead to an actual improvement, on all other targets?
>
> I'm just wondering whether this could expose a performance regression on some other target ...


I see your point now. Of course, if this does change things around it should hopefully be for the better.

Given all the other calls to isAMCompletelyFolded() and so on, I have never been quite sure that other targets ever needed this extra checking based on checking the actual Instructions like SystemZ. That's why I recently proposed guarding this with TTI.LSRWithInstrQueries(), which is still an alternative, I guess. I think Quentin was against this for the sake of out-of-tree targets, IIRC.

Quentin, what is your opinion now?


https://reviews.llvm.org/D35933