[PATCH] D62357: [IRBuilder] Add CreateUnOp(...) and CreateFNegFMF(...)
Cameron McInally via Phabricator via llvm-commits
llvm-commits at lists.llvm.org
Thu May 23 19:06:52 PDT 2019
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
More information about the llvm-commits
mailing list