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

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

kpn added inline comments.


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


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

https://reviews.llvm.org/D53157