[PATCH] D93065: [InstCombine] Disable optimizations of select instructions that causes propagation of poison values

Mon Dec 14 14:34:27 PST 2020

spatel added a comment.

In D93065#2452949 <https://reviews.llvm.org/D93065#2452949>, @congzhe wrote:

> In D93065#2450198 <https://reviews.llvm.org/D93065#2450198>, @nikic wrote:
>
>> This will require other parts of the compiler (especially anything dealing with and/or'd branch conditions, like SCEV, LVI, ValueTracking etc) to understand this new form of and/or first. I don't think most optimizations care about whether and/or is poison-blocking or not, but they need to recognize the select form if we don't canonicalize.
>
> Thank you, I do agree with you that some infrastructures like SCEV might be impacted. Could you clarify it a bit? I am working on unit tests and perf degradation. Should the cause of any of them be those infrastructures, I will post fixes. Would that address your concern? Or something more than that is needed?"
>
> In D93065#2450391 <https://reviews.llvm.org/D93065#2450391>, @spatel wrote:
>
>> 
>
> Thanks Sanjay, by "perf data" I'm wondering which benchmarks does the community mostly care about? LLVM test-suite?

We would get a different answer for each person/company, but test-suite is solid common ground. I think SPEC is also widely used by a large part of the community.

> As you said there's two ways to address this bug -- one is this patch, the other is inserting freezes. With this patch I do see degradations on some internal benchmarks and I'm working on the fix. Do you think it is the right track? I just would like to make sure before going too far.
>
> The work I do is on AArch64. It may or may not impact other architectures (X86, PowerPC, etc). I'm wondering what is the typical way in the community to address performance degradations since addressing degradations for all possible architectures seems significant amount of work hence not likely?

In general, we try to address known regressions in advance. If you see a problem on AArch64, it will probably not be too different for the other CPU targets.
It is acknowledged that it is impossible to know how a change like this will affect everything, so as long as we have a plan to deal with problems, it's fine to proceed.
My suggestion is to compare the regressions between this patch vs. adding `freeze` on test-suite. Is there a clear winner (number and average size of regressions)?
Thank you for pushing this forward!

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

https://reviews.llvm.org/D93065