[PATCH] D84940: [JumpThreading] Conditionally freeze its condition when unfolding select

[Juneyoung Lee via Phabricator via llvm-commits]

aqjune added inline comments.


================
Comment at: llvm/lib/Transforms/Scalar/JumpThreading.cpp:2809
+  if (!InsertFreezeWhenUnfoldingSelect &&
+      BB->getParent()->hasFnAttribute(Attribute::SanitizeMemory))
     return false;
----------------
efriedma wrote:
> eugenis wrote:
> > efriedma wrote:
> > > I suspect we don't want to change this.  Even if "freeze" version of the transform is legal with msan, it probably reduces the quality of msan diagnostics.
> > Actually, I think this would reintroduce https://bugs.llvm.org/show_bug.cgi?id=45220, because the current handling of freeze instruction in MSan is to check (and report) the argument and mark the result as initialized / defined. 
> > 
> > Is this wrong? Should we instead ignore the input shadow in freeze? In that case, I agree, this change will make MSan miss some issues.
> If msan diagnoses uninitialized arguments to freeze, that defeats the point of freeze: the behavior should be defined even if the operand is undef/poison.
> 
> Probably we want to avoid performing transforms that require introducing freeze before msan instrumentation is inserted.  But I think if msan does see a freeze, it's probably better to respect it, as opposed to producing a false-positive diagnostic.
Yes, I agree that msan should ignore the argument of freeze's operand.
BTW, it is interesting that missing alarms from msan can happen with making-program-more-defined transformations. I guess this can happen in other upcoming transformations such as select -> or; it would be great if there is a way of preserving undefinedness of a program for preserving msan alarms.
I'll remove the `!InsertFreezeWhenUnfoldingSelect` here.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D84940