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

[Matt Arsenault via Phabricator via llvm-commits]

arsenm added inline comments.


================
Comment at: llvm/include/llvm/Transforms/IPO/Attributor.h:5120
+
+  bool isAtFixpoint() const override {
+    return Known.Mode.Input != DenormalMode::Dynamic &&
----------------
sameerds wrote:
> Some of my earlier comments are wrong, because I misunderstood the part about dynamic modes. But here's what I think needs to be consistent with the notion of a fixed point. The `update()` calls this function to check if it should attempt a refinement in yet another iteration. But the `updateImpl()` function tries to indicate when a fixed point is reached, which is when it is unable to follow through all the callsites anymore. There's a loss of information about whether future iterations should call `updateImpl()` again, because `indicatePessimisticFixpoint()` doesn't change any state.
> 
> In that case, maybe the state of this Attribute should really be a single bool called `AtFixpoint`? Then both optimistic and pessimistic calls set it to `true`, and return `UNCHANGED`, while `isAtFixpoint()` returns the current value of this bool.
> 
> Also, the introductory comments in Attributor.h should be updated to say that the Known/Assumed scheme is the most common, but not the only way to represent fixed points.
> In that case, maybe the state of this Attribute should really be a single bool called AtFixpoint?

That's what SetState does, I can just copy that


================
Comment at: llvm/lib/Transforms/IPO/AttributorAttributes.cpp:8964
+
+    return Change;
+  }
----------------
sameerds wrote:
> If all modes are "not dynamic", should this call optimistic fixpoint?
This case seems interesting. The other attributes seem to not do that, but it does appear to fire


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

https://reviews.llvm.org/D156129