[PATCH] D138526: [SimpleLoopUnswitch] unswitch selects

Tue May 2 16:26:22 PDT 2023

vitalybuka requested changes to this revision.
vitalybuka added a comment.
This revision now requires changes to proceed.

In D138526#4313843 <https://reviews.llvm.org/D138526#4313843>, @caojoshua wrote:

> I think this patch's transformations are correct, and that the affected test has a true case of use-of-uninitialized-value. I posted a review in https://reviews.llvm.org/D149698. If that patch is merged, I would like to reland this patch.

This could be correct if you ignore other features of compiler, like msan.
Msan goal is to detects UB issues in C/C++ code. So before Msan we can't run transformation which eliminate information about the program. This patch does, so it can't detect UBs correctly.
So I am strongly against landing this patch as-is.

So what can we do about the issue?
It would be nice to change the patch to preserve structure so msan works correctly.
E.g. in order of preference:

1. if possible avoid branching on uninitalized variables, select is fine.
2. If it's hard, the do not run such transformations before msan. Ideally sanitizers should run as earliest passes in pipeline, but it greatly reduces performance of sanitizers (times). So they benefit from the most simplification passes. But then time to time we get into situation like this.
3. If there is a reason to run the pass in exactly that point of pipeline, then don't run transformation if function has "sanitize_memory" attribute. (I don't see implication for other sanitizers from this patch).

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D138526