[PATCH] D69978: Separately track input and output denormal mode

[Matt Arsenault via Phabricator via llvm-commits]

arsenm marked an inline comment as done.
arsenm added inline comments.


================
Comment at: llvm/docs/LangRef.rst:1822
 
 ``"denorm-fp-mode"``
+   This indicates the subnormal handling that may be assumed for the
----------------
andrew.w.kaylor wrote:
> I don't like the definition of this attribute. It's not reader-friendly. The comma-separated pair format has no indication which value refers to inputs and which refers to outputs. Also, while this predates your changes, I think the meanings of the current choices are unclear. 
> 
> What would you think of a comma-separated list with the following possibilities?
> 
> 
> ```
> allow-denormals (default)
> inputs-are-zero (outputs not flushed)
> inputs-are-zero, outputs-are-zero
> inputs-are-zero, outputs-are-positive-zero
> inputs-are-positivezero (outputs not flushed)
> inputs-are-positivezero, outputs-are-zero
> inputs-are-positivezero, outputs-are-positive-zero
> denormal-outputs-are-zero (inputs are unchanged)
> denormal-outputs-are-positive-zero (inputs are unchanged)
> 
> ```
> I'd also be open to abbreviations. I don't know if "daz" and "ftz" are readable to everyone, but I'm more comfortable with them. That would make the options something like this.
> 
> 
> ```
> allow-denormals
> daz
> daz, ftz
> daz, ftz+
> daz+
> daz+, ftz
> daz+, ftz+
> ftz
> ftz+
> ```
I'm trying to avoid needing to autoupgrade bitcode at this point, which leaving the names as-is accomplishes. I'm worried this could still end up not in the right place, and then we would need another level of auto upgrade to deal with it later. I think these are overly verbose (I'm also keeping in mind the fact that any use of these does a linear scan through all string attributes, and then needs to parse these).

I'm also unclear on what this weird ARM positive-zero really means. Does it mean inputs and outputs ignored the sign? Is there value in representing positive-zero on both sides?


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

https://reviews.llvm.org/D69978