[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:5114
+
+  ChangeStatus indicateOptimisticFixpoint() override {
+    return ChangeStatus::UNCHANGED;
----------------
sameerds wrote:
> sameerds wrote:
> > arsenm wrote:
> > > sameerds wrote:
> > > > Is it correct to say that an optimistic fix point is when all the "dynamic" components are set to default (`ieee`)? This attribute is reinventing what a fixed point means. Does this function need to set the optimistic fixed point? Or I would guess this function is actually never called, in which case it should be marked with `llvm_unreachable()`?
> > > No. You cannot say IEEE is more optimistic than preserve-sign or positive-zero. They're just different. The only thing you can infer is known or unknown
> > I understand what you want it to do. I am trying to understand what the code actually means. The manifest function replaces "dynamic" with "default/ieee". Both the optimistic and pessimistic fixed points leave dynamic alone. What is a fixed point here? When a client calls either of the fixed point functions, what should the interpretation of "dynamic" be? Should it be the same as the manifest method, which means setting it to default/ieee? Clearly, the client intended that no more changes should be possible, but the manifest will change it.
> Or to put it in more concrete terms, is it okay that either of the following patterns, if it occurs, can potentially fire the assert?
> 
> ```
> indicateOptimisticFixpoint();
> assert(isAtFixpoint());
> ```
> and
> ```
> indicatePessimisticFixpoint();
> assert(isAtFixpoint());
> ```
I believe isValidState is supposed to protect from this, combined with going to invalid for combinations that don't make sense. Combinations that don't make sense go to invalid and then should be ignored


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

https://reviews.llvm.org/D156129