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

[Sameer Sahasrabuddhe via Phabricator via llvm-commits]

sameerds added inline comments.


================
Comment at: llvm/include/llvm/Transforms/IPO/Attributor.h:5114
+
+  ChangeStatus indicateOptimisticFixpoint() override {
+    return ChangeStatus::UNCHANGED;
----------------
arsenm wrote:
> 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
I am sorry I don't follow that at all. The attribute has a definition for //a// fixed point. I understand setting nonsensical combinations to invalid state, but the set-fixed-point functions neither set the invalid state, nor some state that satisfies `isAtFixPoint()`. Then how are the above asserts prevented?

Taking a step back, I am not opposed to the goal being achieved here, but I am not seeing how it fits the apparent design of the Attributor. Should this attribute set a new precedent for what a fixed point means? I would rather defer to others who are more familiar with the original intention of the Attributor.


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

https://reviews.llvm.org/D156129