[PATCH] D53157: Teach the IRBuilder about constrained fadd and friends

[Kevin P. Neal via Phabricator via cfe-commits]

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


================
Comment at: include/llvm/IR/IRBuilder.h:1138
+
+    return MetadataAsValue::get(Context, RoundingMDS);
+  }
----------------
rjmccall wrote:
> kpn wrote:
> > rjmccall wrote:
> > > Huh?  You build an `MDNode` that wraps an `MDString` and then immediately extract the `MDString` from it and drop the `MDNode`?
> > > 
> > > I think you should just have a function somewhere that returns the correct metadata string for a `ConstrainedRoundingKind`, and then the code here is much more obvious.  Maybe this can be wherever you declare the enums.  You can also have a function that goes the other way and returns an `Optional<ConstrainedRoundingKind>`.
> > > 
> > > Function name should include `FP`.
> > > 
> > > Same points apply to the next function.
> > Well, when you put it that way, your proposed change does result in much simpler code.
> > 
> > The resulting code isn't that much more than the switch. I hope it is now easy enough to read. These functions are private to the class and just exist to be helpers. I don't see a need to put the switch table into a different function, but I'm willing to be convinced. 
> I feel like having the enum->string mapping available somewhere independent of `IRBuilder` will probably be useful to someone, and it nicely separates concerns in any case .  You could just make it a static method on `ConstrainedFPIntrinsic` along with the reverse operation.
Ok, will do.


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

https://reviews.llvm.org/D53157