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

[Adam Nemet via Phabricator via llvm-commits]

anemet marked 2 inline comments as done.
anemet 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 {
----------------
spatel wrote:
> 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.
Yes, that is why I added the comment.  You're right I mixed up the name of the field.


================
Comment at: include/llvm/IR/Operator.h:209-211
+  void setAllowContract(bool B) {
+    Flags = (Flags & ~AllowContract) | B * AllowContract;
+  }
----------------
spatel wrote:
> 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.
Setting it with false is used in the clang patch in https://reviews.llvm.org/D31168.

But yes, I am planning to change all of these as a follow-on.  I was going to put it in this patchset but the change is actually larger than I expected.


https://reviews.llvm.org/D31164