[PATCH] D85759: [SLPVectorizer] Fix regression from cost model refactoring

Thomas Lively via Phabricator via llvm-commits llvm-commits at lists.llvm.org
Thu Aug 13 14:29:17 PDT 2020


tlively added a comment.

In D85759#2212136 <https://reviews.llvm.org/D85759#2212136>, @aheejin wrote:

> I think D79941 <https://reviews.llvm.org/D79941> is //supposed// to work; what it did is move those logics into constructors of `IntrinsicCostAttributes`. For example, this constructor <https://github.com/llvm/llvm-project/blob/b0b95dab1ce21d93f3d62bc37256da9f38cff616/llvm/lib/Analysis/TargetTransformInfo.cpp#L93-L106> saves `FMF` and argument types, which are supposed to be used later in the same way. I think it'd be better to figure that out and fix it than restore the old code? Maybe the original author @samparker has some ideas on why :)

Before the original change and after this patch, the intrinsic cost calculation here uses the type-based path. Using the constructor you pointed out is what the regressed code did, and it changed the behavior to use the argument-based path because that constructor also records the argument values. There is no constructor available that encapsulates this exact logic but doesn't also record the argument values.

In D85759#2212471 <https://reviews.llvm.org/D85759#2212471>, @samparker wrote:

> I know what @sanwou01 saw an important uplift from this change though, so it would be nice if we could maintain that (not sure if he committed a test) and fix this regression.

I'm not sure we should be trying to preserve an improvement that the previous change introduced, given that it was meant to be a non-functional change. There doesn't seem to be a test for it, anyway. On the flip side, I could also probably fix the WebAssembly regression by adding logic to the WebAssembly TTI if folks think that this fix is a code maintenance regression and that it's ok that 8cc911fa5b06 <https://reviews.llvm.org/rG8cc911fa5b06f055430d006d0640248fcac2a101> was a functional change after all.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D85759



More information about the llvm-commits mailing list