[PATCH] D96399: [X86][CodeGenPrepare] Try to reuse IV's incremented value instead of adding the offset

Thu Mar 4 11:47:59 PST 2021

nikic added a comment.

In D96399#2604109 <https://reviews.llvm.org/D96399#2604109>, @reames wrote:

> In D96399#2604070 <https://reviews.llvm.org/D96399#2604070>, @nikic wrote:
>
>> For most programs the regression is resolved, only mafft still has a noteworthy regression, as well as the ReleaseLTO-g configurations (don't know if related to the LTO part or the `-g` part though).
>
> How concerned by this are you?  To me this looks relatively isolated, and likely directly related to code changes in the respective benchmarks.  (Though, is that an easy way to check that without building it all myself?)  I can see one more small tweak I can make to defer domtree usage a bit further (will do so), but at some point it's pretty fundamental to this change (which I do think is worthwhile).  Thoughts?

I'm not very concerned about that particular regression, but rather the potential implications. If this is indeed still caused by domtree calculations (rather than second order effects -- that also sounds plausible), then it is quite likely that there are also pathological cases (this is a typical issue for domtree invalidation + use in the same pass).

> As an aside, the analysis invalidation in this code is a mess.  We appear to be leaving loopinfo (which is derived from DT) stale.  It just happens that the particular variety of stale doesn't expose problems in practice (joy).  It's also not clear why we don't just update DT in the various transforms which modify it.  None of them appear particularly hard to do.

Yeah, it's a real mess. What I find particularly concerning is that a lot of transforms set a ModifiedDT flag, despite not doing any CFG changes! For example combineToUAddWithOverflow() seems to use this flag to avoid instruction iterator invalidation -- flushing the DT and reprocessing the whole function is a pretty big hammer for that problem.

Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D96399/new/

https://reviews.llvm.org/D96399