[PATCH] D138526: [SimpleLoopUnswitch] unswitch selects

Mon May 8 13:18:07 PDT 2023

caojoshua added a comment.

> Freeze is going to be false negatives, which are bad for msan, but better then false positives. Taking that this is -O3 only the patch is LGTM.
> I played with existing version of compiler, looks like freeze from this pass can hide a lot of bugs.
> So please consider just make this patch NOOP if function has sanitize_memory attribute. Do you see disadvantages in this?

Could you share an example of a case where LoopUnswitch could hide a bug? I'm guessing its where we hoist a branch that could be undef. We insert a freeze that hides the use of undef, but maybe its actually a true undef and we get a false negative. I'm having some trouble producing this case atm.

Not sure what examples you were able to produce, but "trivial" loop unswitching is enabled in O1 <https://github.com/llvm/llvm-project/blob/14971ae255a4ab5f7d569247f55083923bfa5f2b/llvm/lib/Passes/PassBuilderPipelines.cpp#L435>. It is possible that even if we switch msan builders to a lower optimization level, some bugs could still be hidden.

>From my understanding, you found the pass as a whole, and not just this patch, can hide bugs. If we filter by `sanitize_memory`, we should probably turn it on for the entire pass, and not just this patch. I would be OK to add this change in a separate patch.

> This is beyond my expertise. I'll accept, but please set a blocking reviewer to catch attention of particular person.

I added @nikic as a blocking reviewer.

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D138526