[PATCH] D102834: [SLP] Implement initial memory versioning.

Thu Dec 16 02:07:08 PST 2021

SjoerdMeijer added a comment.

In D102834#3197066 <https://reviews.llvm.org/D102834#3197066>, @fhahn wrote:

> In D102834#3175761 <https://reviews.llvm.org/D102834#3175761>, @SjoerdMeijer wrote:
>
>> Yeah okay, sure, fair enough. Just wanted to double check, and you haven't seen any performance disasters in Spec or the test suite?
>> I am thinking if we should not just start with this enabled:
>>
>>   static cl::opt<bool> EnableMemoryVersioning(
>>       "slp-memory-versioning", cl::init(false), cl::Hidden,
>>
>> If we get perf regression reports, this can be kept in tree but with this flag off, then at least we directly know what to fix.
>
> I agree it would be good to start with this enabled by default! My main concern at the moment is compile-time impact. I need to re-measure, but from last time I remember that there were a few CTMark cases that had noticeable increases.

Ok, that would indeed be good to double-check.

> I think the rate of when versioning leads to actually vectorizing the versioned block at the moment is ~15%. I think for it to be enabled by default it would be good to increase this rate first. At the moment the reason this rate is so low is that we consider a block for versioning once we found an aliasing access and effectively stop analyzing further. So even if there are other problems preventing vectorization, we still try to version it.

Ok, I see. I suspect there will be a bit of a long tail of work anyway. But as this is such a crucial enabler for any further work in this area, I would be inclined to start with this on by default and "see what happens". If it manages to be enabled by default, that would be so much more ideal...

I am going to look over this one more time, but think I am going to LGTM this soon (requesting another lgtm too).

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D102834