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

[John McCall via Phabricator via cfe-commits]

rjmccall added inline comments.


================
Comment at: include/llvm/IR/IRBuilder.h:1138
+
+    return MetadataAsValue::get(Context, RoundingMDS);
+  }
----------------
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.


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

https://reviews.llvm.org/D53157