[PATCH] D69598: Work on cleaning up denormal mode handling

[Matt Arsenault via Phabricator via llvm-commits]

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


================
Comment at: clang/lib/CodeGen/CGCall.cpp:1736-1737
+    if (CodeGenOpts.FPSubnormalMode != llvm::SubnormalMode::Invalid)
+      FuncAttrs.addAttribute("denormal-fp-math",
+                             llvm::subnormalModeName(CodeGenOpts.FPSubnormalMode));
 
----------------
spatel wrote:
> arsenm wrote:
> > spatel wrote:
> > > Do you plan to change the attribute string from "denormal" to "subnormal" as part of upgrading it to work per-FP-type? Would we need to auto-upgrade old IR as part of making the string consistent with the code?
> > > 
> > > Can we stash the attribute string name inside a getter function in the new ADT file, so clang and LLVM have a common source of truth for the attribute name?
> > I'm considering it, but at the moment I'm trying to avoid changes. The next step I'm working on is adding denormal-fp-math-f32 (or maybe subnormal-fp-math-f32), which will co-exist and override the current attribute if the type matches
> I think it would be better to not change the vocabulary incrementally then. Ie, keep everything "denormal" in this patch, and then universally change the terminology to "subnormal" in one step. That way we won't have any inconsistency/confusion between the attribute name and the code.
In the follow up patch, the new attribute uses the old denormal  name. The clang option handling maintains the old name to match the flag, but the new internal  enums and functions use the subnormal name. Is that a reasonable state? I don’t want to spread the old name through the new utilities, but also don’t want to have to auto upgrade bitcode for a name change 


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

https://reviews.llvm.org/D69598