[PATCH] D156129: Attributor: Try to propagate concrete denormal-fp-math{-f32}

[Matt Arsenault via Phabricator via llvm-commits]

arsenm marked 2 inline comments as done.
arsenm added inline comments.


================
Comment at: llvm/include/llvm/Transforms/IPO/Attributor.h:5114-5116
+    bool operator!=(const DenormalState Other) const {
+      return Mode != Other.Mode || ModeF32 != Other.ModeF32;
+    }
----------------
sameerds wrote:
> The usual way is to just invert the `==` operator?
I will never understand why C++ doesn't do this for you


================
Comment at: llvm/include/llvm/Transforms/IPO/Attributor.h:6317-6320
   /// This function should return true if the type of the \p AA is
   /// AAIndirectCallInfo
+  /// This function should return true if the type of the \p AA is
+  /// AADenormalFPMath.
----------------
sameerds wrote:
> Is this just a quirk of how Phab displays the diff? Or the new struct started at the wrong point?
I think this is a consequence of diffs based on multiple starting points. On the last rebase a few new AAs showed up and pushed this further down the file

Plus the operator<< here was misplaced to begin with, it should be moved elsewhere


================
Comment at: llvm/include/llvm/Transforms/IPO/Attributor.h:5063-5066
+      if (Caller.ModeF32 == DenormalMode::getInvalid())
+        Caller.ModeF32 = Caller.Mode;
+      if (Callee.ModeF32 == DenormalMode::getInvalid())
+        Callee.ModeF32 = Callee.Mode;
----------------
arsenm wrote:
> arsenm wrote:
> > It turns out moving this to handle the invalid mode at initialize broke the partially dynamic mode case from a fixed caller (and my test for that was accidentally dead and deleted). At this point I'll fix that separately if it matters
> And by if it matters, I mean it doesn't now because the libraries are compiled with full denormal-fp-math=dynamic. We probably should refine that to only the f32, because the f64/f16 flushing probably does not work in practice
I looked at this again and the case that regressed is invalid, we just now get worse handling of invalid cases which is not interesting


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

https://reviews.llvm.org/D156129