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

Mon Dec 14 13:15:12 PST 2020

congzhe added a comment.

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:

> You will also need to update several regression tests (`make check` or `ninja check`) to show new canonical form.
> I'm still not sure which path (this or insert freezes) is easier/better - if we have perf data, maybe it will tell us how to go?

Thanks Sanjay, by "perf data" I'm wondering which benchmarks does the community mostly care about? LLVM test-suite?

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?

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

https://reviews.llvm.org/D93065