[PATCH] D31164: [IR] Add AllowContract to FastMathFlags

[Sanjay Patel via Phabricator via llvm-commits]

spatel added inline comments.


================
Comment at: include/llvm/IR/Operator.h:177-178
 public:
+  /// This is how the bits are used in Value::SubclassData so they should fit
+  /// there too.
   enum {
----------------
This comment isn't right - FMF is using "SubclassOptionalData" not "SubclassData".

Not sure if it's worth mentioning as a code comment, but we only have room for one more FMF bit if we keep using SubclassOptionalData; it only has 7 bits total.


================
Comment at: include/llvm/IR/Operator.h:209-211
+  void setAllowContract(bool B) {
+    Flags = (Flags & ~AllowContract) | B * AllowContract;
+  }
----------------
If we want to add a bool param, should we do that for all of the flags as a follow-up change? Seems odd to make setAllowContract() different than everything else if there's no change to use a 'false' setter in this patch.


================
Comment at: include/llvm/IR/Operator.h:319-320
 
+  /// Test whether this operation is permitted to
+  /// be floating-point contracted, aka the 'C' fast-math property.
+  bool hasAllowContract() const {
----------------
I know it's copying the format of the other comments, but when do we use these single-letter abbreviations of the FMF? If never, just remove these comments?


https://reviews.llvm.org/D31164