[PATCH] D62357: [IRBuilder] Add CreateUnOp(...) and CreateFNegFMF(...)

[Cameron McInally via Phabricator via llvm-commits]

cameron.mcinally added inline comments.


================
Comment at: llvm/include/llvm/IR/IRBuilder.h:1369
                     MDNode *FPMathTag = nullptr) {
-    if (auto *VC = dyn_cast<Constant>(V))
-      return Insert(Folder.CreateFNeg(VC), Name);
+    if (Value *VC = foldConstant(Instruction::FNeg, V, Name)) return VC;
     return Insert(setFPAttrs(BinaryOperator::CreateFNeg(V), FPMathTag, FMF),
----------------
craig.topper wrote:
> Why can't this keep using Folder.CreateFNeg? CreateNot uses Folder.CreateNot.
I considered this also and went with something more like CreateBinOp (see CreateUnOp too). The UnOps are odd ducks since they’re not really unary operators, but rather they’re implemented with existing BinOps. IMO, we should be moving away from the weirdness of the UnOps...

I don’t feel very strongly about this though. If you think the current UnOp direction is better, that’s fine with me. 


================
Comment at: llvm/include/llvm/IR/IRBuilder.h:1379
+   if (Value *VC = foldConstant(Instruction::FNeg, V, Name)) return VC;
+   return Insert(setFPAttrs(UnaryOperator::CreateFNeg(V), nullptr,
+                            FMFSource->getFastMathFlags()),
----------------
craig.topper wrote:
> It feels weird that this creates unaryoperator::fneg but CreateFNeg uses the binaryoperator. Shouldn't these be consistent until we switch over?
So I did consider that. My thinking was that we wouldn’t be regressing calling the UnaryOperator here, since it’s brand new code, so it’s better to move towards the goal. The reason we haven’t updated the old BinaryOperator code yet is that we’re afraid of regressions.

I do see your point though...

Also, let me be clear that this function will have at least one use in a patch to come. I’ve just broken out this digestible chunk for now. 


Repository:
  rL LLVM

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

https://reviews.llvm.org/D62357