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

[Congzhe Cao via Phabricator via llvm-commits]

congzhe added a comment.

In D93065#2453190 <https://reviews.llvm.org/D93065#2453190>, @spatel wrote:

> 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!

On test suite it seems we did not have a clear winner.

Out of ~3600 microbenchmarks I choose the ones with running time greater than 4us, that gives me ~250 benchmarks. Regarding the overall geometric mean difference to llvm trunk code,  both the freeze solution and this patch have less than 0.7% difference to trunk. We have 9 microbenchmarks that degrades over 2% for the freeze solution where the geomean for those 9 degradations is 3.7%; for this patch we have 12 microbenchmarks that degrades over 2% for the freeze solution where the geomean for those 12 degradations is 4.3%.

If I choose the microbenchmarks  with running time greater than 0.5us, that gives me ~400 microbenchmarks where I have similar observations.

However, on internal benchmarks I see much less degradation from the freeze solution compared to this patch.

Does this sound like a clue that we should go ahead with the freeze solution? If so I'll post the freeze solution and see how we can go from there.


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

https://reviews.llvm.org/D93065