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

[Cameron McInally via Phabricator via llvm-commits]

cameron.mcinally marked 2 inline comments as done.
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),
----------------
cameron.mcinally wrote:
> cameron.mcinally wrote:
> > 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. 
> Oh oh, I just remembered why I went down the BinOps path. If I’m not mistaken, the Folder.CreateFNeg method throws away FMF when there’s no folder. I wanted to avoid that. I’ll confirm in the morning...
> If I’m not mistaken, the Folder.CreateFNeg method throws away FMF when there’s no folder.

I confused myself. That is a problem, but it's not this problem. In general, when the NoFolder is used, the new `CreateF*FMF(...)` instruction picks up the default FMFs from the IRBuilder, not the instruction it's supposed to copy from.

So, yeah, I could change this code back to make it look more like CreateNot, if that's the preference.


================
Comment at: llvm/unittests/IR/PatternMatch.cpp:641
+  EXPECT_EQ(LoadInst, Match);
 }
 
----------------
Just realized I misplaced these tests, so I'll have to update them.


Repository:
  rL LLVM

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

https://reviews.llvm.org/D62357