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

Evgenii Stepanov via Phabricator via llvm-commits llvm-commits at lists.llvm.org
Mon Aug 3 11:07:30 PDT 2020


eugenis added a subscriber: guiand.
eugenis added inline comments.


================
Comment at: llvm/lib/Transforms/Scalar/JumpThreading.cpp:2809
+  if (!InsertFreezeWhenUnfoldingSelect &&
+      BB->getParent()->hasFnAttribute(Attribute::SanitizeMemory))
     return false;
----------------
aqjune wrote:
> 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.
FYI @guiand fixed MSan in D85040. It does not change anything for this change, which almost LGTM. Please update the comment to say that this transformation would reduce the quality of msan diagnostics instead of introducing UB.



Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D84940



More information about the llvm-commits mailing list