Support FNeg ConstantExpr folding

[cameron.mcinally]

Summary

Also, fix ConstantExpr::getFNeg(...) so that it actually returns an FNeg(X), not an FSub(-0.0, X).

[arsenm]

llvm/lib/IR/ConstantFold.cpp

948–949 ↗

(On Diff #197693)

You can use neg(CV) and remove C1V

[arsenm]

llvm/test/CodeGen/X86/fp-fold.ll

226–242 ↗

(On Diff #197693)

These tests don't really belong in codegen (same for the vector cases).

Can you also add a test with some weird constant expressions as a source?

[cameron.mcinally]

Use neg(...) instead of changeSign().

[cameron.mcinally]

llvm/test/CodeGen/X86/fp-fold.ll

226–242 ↗

(On Diff #197693)

These tests don't really belong in codegen (same for the vector cases).

I'm not that familiar with LLVM's testing layout. Where should they go?

Can you also add a test with some weird constant expressions as a source?

Do you mean like the 42.0 I see everywhere? Or something more exotic?

There are existing tests that exercise NaN, Inf, and undef in place now. Those came for free from the old FNEG(X) -> FSUB(-0.0, X) work.

[arsenm]

llvm/test/CodeGen/X86/fp-fold.ll

226–242 ↗

(On Diff #197693)

Looks like test/Analysis/ConstantFolding is probably the right place.

I mean something like a bitcast of a global address, that isa<Constant>, but isn't ConstantFP

[spatel]

Let me know if I'm not seeing it correctly, but the DAG change is independent of the IR changes, so it should be a stand-alone patch.

[cameron.mcinally]

llvm/test/CodeGen/X86/fp-fold.ll

226–242 ↗

(On Diff #197693)

Sorry, my ignorance is showing here...

Looks like test/Analysis/ConstantFolding is probably the right place.

It looks like all the RUN: lines in that directory invoke opt with a specific pass or general optimization level. I think I need llc to trigger the constant folder for this patch. Is that correct? Or am I missing something obvious...

[arsenm]

llvm/test/CodeGen/X86/fp-fold.ll

226–242 ↗

(On Diff #197693)

llc should definitely not be needed.

I would expect -instsimplify to work.

[cameron.mcinally]

In D61419#1487834, @spatel wrote:

Let me know if I'm not seeing it correctly, but the DAG change is independent of the IR changes, so it should be a stand-alone patch.

Yes, you're correct. Kind of... I noticed that FNeg wasn't folding vectors with some undef elements correctly while working on this change. I can separate that patch if you'd like.

[cameron.mcinally]

llvm/test/CodeGen/X86/fp-fold.ll

226–242 ↗

(On Diff #197693)

I would expect -instsimplify to work.

It does not. Not even with -O3.

This is probably a problem with my understanding of Constant Folding. The code I added only folds constants when a new ConstantExpr is created. I don't see any hooks for an opt pass to do folding on the fly though. I'll keep digging to see if I can find my problem...

Or maybe I should just be writing more complex tests that regenerate the FNeg after one of its operands are optimized. I'm not sure...

[cameron.mcinally]

llvm/test/CodeGen/X86/fp-fold.ll

226–242 ↗

(On Diff #197693)

I would expect ` to work

Ah, ok. So the instsimplify pass does it's own folding apart from ConstantExpr. That code will need updating too...

[cameron.mcinally]

Couple of updates...

In D61419#1487929, @cameron.mcinally wrote:

In D61419#1487834, @spatel wrote:

Let me know if I'm not seeing it correctly, but the DAG change is independent of the IR changes, so it should be a stand-alone patch.

Yes, you're correct. Kind of... I noticed that FNeg wasn't folding vectors with some undef elements correctly while working on this change. I can separate that patch if you'd like.

This actually can't be separated out. We were mistakenly generating an FSub, instead of an FNeg in the IRBuilder, so the DAG code won't trigger without the IRBuilder change. I won't be able to write a test for it without that change (at least that I know about right now) .

I would expect -instsimplify to work.

@arsenm, the InstructionSimplify pass has a whole different sets of folds from the ConstantExpr folds. Piling on top of that, we'll have to add support for UnaryInstructions to the other passes that make use of InstructionSimplify. That seems like a big change. @kpn is also working on D61447, which I just found out about. I propose adding the ConstantExpr folding changes as-is, with the expectation that InstructionSimplify folding (and probably other places that I don't know about) will have to be done too. Does anyone feel strongly against this?

[cameron.mcinally]

In D61419#1488170, @cameron.mcinally wrote:

Couple of updates...

In D61419#1487929, @cameron.mcinally wrote:

In D61419#1487834, @spatel wrote:

Let me know if I'm not seeing it correctly, but the DAG change is independent of the IR changes, so it should be a stand-alone patch.

Yes, you're correct. Kind of... I noticed that FNeg wasn't folding vectors with some undef elements correctly while working on this change. I can separate that patch if you'd like.

This actually can't be separated out. We were mistakenly generating an FSub, instead of an FNeg in the IRBuilder, so the DAG code won't trigger without the IRBuilder change. I won't be able to write a test for it without that change (at least that I know about right now) .

Oh, I suppose I could let the DAG change sit until after the IRBuilder change lands also.

[cameron.mcinally]

Remove DAG changes as @spatel suggested.

[cameron.mcinally]

In D61419#1488170, @cameron.mcinally wrote:

This actually can't be separated out. We were mistakenly generating an FSub, instead of an FNeg in the IRBuilder, so the DAG code won't trigger without the IRBuilder change. I won't be able to write a test for it without that change (at least that I know about right now) .

I would expect -instsimplify to work.

@arsenm, the InstructionSimplify pass has a whole different sets of folds from the ConstantExpr folds. Piling on top of that, we'll have to add support for UnaryInstructions to the other passes that make use of InstructionSimplify. That seems like a big change. @kpn is also working on D61447, which I just found out about. I propose adding the ConstantExpr folding changes as-is, with the expectation that InstructionSimplify folding (and probably other places that I don't know about) will have to be done too. Does anyone feel strongly against this?

I'll retract this statement. The InstructionSimplify work wasn't as bad as it first appeared. There are users of InstructionSimplify that will need to be updated to take advance of the UnaryOps, but those don't have to be done right away.

Patch incoming...

[cameron.mcinally]

Add support for FNeg constant folding in InstructionSimplify.

[cameron.mcinally]

Clean up PatternMatch.h logic and capture all of test/Analysis/ConstantFolding/fneg.ll

[arsenm]

llvm/test/CodeGen/X86/vec_fneg.ll

144 ↗

(On Diff #197904)

These should still not be codegen tests. Is this the constant folding that the asm parser does?

[cameron.mcinally]

llvm/test/CodeGen/X86/vec_fneg.ll

144 ↗

(On Diff #197904)

The folding I'd like to test is done whenever a ConstantExpr::get(...) or ConstantExpr::getFNeg(...) is called. So it's really all over the place. A quick grep shows it's mainly called in IR, but also in Analysis, Transforms, CodeGen, and the AsmParser.

unittests/IR/ConstantsTest.cpp seems like a candidate, but this doesn't really fit a unit test.

There is some prior art in CodeGen for constant folding tests, but I'm not sure what constant folder they're testing. E.g. @div_select_constant_fold(...):llvm/test/CodeGen/X86/fdiv-combine.ll.

[arsenm]

llvm/test/CodeGen/X86/vec_fneg.ll

144 ↗

(On Diff #197904)

If any codegen tests are testing the IR constant folding, that's just broken. I would try to just use the asm parser for this

[cameron.mcinally]

llvm/test/CodeGen/X86/vec_fneg.ll

144 ↗

(On Diff #197904)

I actually just shot a question out to llvm-dev about this. I'll wait to see if anyone knows for sure before making that change. Thanks, Matt.

[spatel]

There are still independent changes here from what I can tell.
Can we start with this: extract the pattern match diff and add a test for it to unittests/IR/PatternMatch.cpp ?

[cameron.mcinally]

llvm/test/CodeGen/X86/fp-fold.ll

226–242 ↗

(On Diff #197693)

Bah! Apologies, Matt. You were right all along. SimplifyFNegInst(...) in InstructionSimplify.cpp calls ConstantFoldUnaryOpOperand(...), which eventually ends up in ConstantExpr::get(...). So testing with -instsimplify is enough to exercise these new folds.

I'll remove these tests since they're already covered in Analysis/ConstantFolding/fneg-instcombine.ll.

[cameron.mcinally]

Remove misplaced test/CodeGen tests.

Also add support for UnOps to ConstantFoldInstOperandsImpl(...):lib/Analysis/ConstantFolding.cpp.

[cameron.mcinally]

In D61419#1489734, @spatel wrote:

There are still independent changes here from what I can tell.
Can we start with this: extract the pattern match diff and add a test for it to unittests/IR/PatternMatch.cpp ?

That's fair. This Diff grew too quickly in too many directions. Maybe we should start fresh in smaller chunks.

PatternMatcher patch coming soon...

[cameron.mcinally]

This comment was removed by cameron.mcinally.

[cameron.mcinally]

This patch was broken up into D61520, D61544, and D61573.

llvm/include/llvm/Analysis/InstructionSimplify.h

SimplifyQuery getWithInstruction(Instruction *I) const {
     return Copy;
   }
 };
 
 // NOTE: the explicit multiple argument versions of these functions are
 // deprecated.
 // Please use the SimplifyQuery versions in new code.
 
+/// Given operand for an FNeg, fold the result or return null.
+Value *SimplifyFNegInst(Value *Op, FastMathFlags FMF,
+                        const SimplifyQuery &Q);
+
 /// Given operands for an Add, fold the result or return null.
 Value *SimplifyAddInst(Value *LHS, Value *RHS, bool isNSW, bool isNUW,
                        const SimplifyQuery &Q);
 
 /// Given operands for a Sub, fold the result or return null.
 Value *SimplifySubInst(Value *LHS, Value *RHS, bool isNSW, bool isNUW,
                        const SimplifyQuery &Q);
 
Value *SimplifyShuffleVectorInst(Value *Op0, Value *Op1, Constant *Mask,
                                  Type *RetTy, const SimplifyQuery &Q);
 
 //=== Helper functions for higher up the class hierarchy.
 
 /// Given operands for a CmpInst, fold the result or return null.
 Value *SimplifyCmpInst(unsigned Predicate, Value *LHS, Value *RHS,
                        const SimplifyQuery &Q);
 
+/// Given operand for a UnaryOperator, fold the result or return null.
+Value *SimplifyUnOp(unsigned Opcode, Value *Op, const SimplifyQuery &Q);
+
+/// Given operand for an FP UnaryOperator, fold the result or return null.
+/// In contrast to SimplifyUnOp, try to use FastMathFlag when folding the
+/// result. In case we don't need FastMathFlags, simply fall to SimplifyUnOp.
+Value *SimplifyFPUnOp(unsigned Opcode, Value *Op, FastMathFlags FMF,
+                      const SimplifyQuery &Q);
+
 /// Given operands for a BinaryOperator, fold the result or return null.
 Value *SimplifyBinOp(unsigned Opcode, Value *LHS, Value *RHS,
                      const SimplifyQuery &Q);
 
 /// Given operands for an FP BinaryOperator, fold the result or return null.
 /// In contrast to SimplifyBinOp, try to use FastMathFlag when folding the
 /// result. In case we don't need FastMathFlags, simply fall to SimplifyBinOp.
 Value *SimplifyFPBinOp(unsigned Opcode, Value *LHS, Value *RHS,

llvm/lib/Analysis/InstructionSimplify.cpp

 #define DEBUG_TYPE "instsimplify"
 
 enum { RecursionLimit = 3 };
 
 STATISTIC(NumExpand,  "Number of expansions");
 STATISTIC(NumReassoc, "Number of reassociations");
 
 static Value *SimplifyAndInst(Value *, Value *, const SimplifyQuery &, unsigned);
+static Value *SimplifyUnOp(unsigned, Value *, const SimplifyQuery &, unsigned);
+static Value *SimplifyFPUnOp(unsigned, Value *, const FastMathFlags &,
+                             const SimplifyQuery &, unsigned);
 static Value *SimplifyBinOp(unsigned, Value *, Value *, const SimplifyQuery &,
                             unsigned);
 static Value *SimplifyFPBinOp(unsigned, Value *, Value *, const FastMathFlags &,
                               const SimplifyQuery &, unsigned);
 static Value *SimplifyCmpInst(unsigned, Value *, Value *, const SimplifyQuery &,
                               unsigned);
 static Value *SimplifyICmpInst(unsigned Predicate, Value *LHS, Value *RHS,
                                const SimplifyQuery &Q, unsigned MaxRecurse);

 }
 
 /// Given operands for a ShuffleVectorInst, fold the result or return null.
 Value *llvm::SimplifyShuffleVectorInst(Value *Op0, Value *Op1, Constant *Mask,
                                        Type *RetTy, const SimplifyQuery &Q) {
   return ::SimplifyShuffleVectorInst(Op0, Op1, Mask, RetTy, Q, RecursionLimit);
 }
 
+static Constant *foldConstant(Instruction::UnaryOps Opcode,
+                              Value *&Op, const SimplifyQuery &Q) {
+  if (auto *C = dyn_cast<Constant>(Op))
+    return ConstantFoldUnaryOpOperand(Opcode, C, Q.DL);
+  return nullptr;
+}
+
+/// Given the operand for an FNeg, see if we can fold the result.  If not, this
+/// returns null.
+static Value *SimplifyFNegInst(Value *Op, FastMathFlags FMF,
+                               const SimplifyQuery &Q, unsigned MaxRecurse) {
+  if (Constant *C = foldConstant(Instruction::FNeg, Op, Q))
+    return C;
+
+  Value *X;
+  // fneg (fneg X) ==> X
+  if (match(Op, m_FNeg(m_Value(X))))
+    return X;
+
+  return nullptr;
+}
+
+Value *llvm::SimplifyFNegInst(Value *Op, FastMathFlags FMF,
+                              const SimplifyQuery &Q) {
+  return ::SimplifyFNegInst(Op, FMF, Q, RecursionLimit);
+}
+
 static Constant *propagateNaN(Constant *In) {
   // If the input is a vector with undef elements, just return a default NaN.
   if (!In->isNaN())
     return ConstantFP::getNaN(In->getType());
 
   // Propagate the existing NaN constant when possible.
   // TODO: Should we quiet a signaling NaN?
   return In;

 
 Value *llvm::SimplifyFRemInst(Value *Op0, Value *Op1, FastMathFlags FMF,
                               const SimplifyQuery &Q) {
   return ::SimplifyFRemInst(Op0, Op1, FMF, Q, RecursionLimit);
 }
 
 //=== Helper functions for higher up the class hierarchy.
 
+/// Given the operand for a UnaryOperator, see if we can fold the result.
+/// If not, this returns null.
+static Value *SimplifyUnOp(unsigned Opcode, Value *Op, const SimplifyQuery &Q,
+                           unsigned MaxRecurse) {
+  switch (Opcode) {
+  case Instruction::FNeg:
+    return SimplifyFNegInst(Op, FastMathFlags(), Q, MaxRecurse);
+  default:
+    llvm_unreachable("Unexpected opcode");
+  }
+}
+
+/// Given the operand for a UnaryOperator, see if we can fold the result.
+/// If not, this returns null.
+/// In contrast to SimplifyUnOp, try to use FastMathFlag when folding the
+/// result. In case we don't need FastMathFlags, simply fall to SimplifyUnOp.
+static Value *SimplifyFPUnOp(unsigned Opcode, Value *Op,
+                             const FastMathFlags &FMF,
+                             const SimplifyQuery &Q, unsigned MaxRecurse) {
+  switch (Opcode) {
+  case Instruction::FNeg:
+    return SimplifyFNegInst(Op, FMF, Q, MaxRecurse);
+  default:
+    return SimplifyUnOp(Opcode, Op, Q, MaxRecurse);
+  }
+}
+
+Value *llvm::SimplifyFPUnOp(unsigned Opcode, Value *Op, FastMathFlags FMF,
+                            const SimplifyQuery &Q) {
+  return ::SimplifyFPUnOp(Opcode, Op, FMF, Q, RecursionLimit);
+}
+
 /// Given operands for a BinaryOperator, see if we can fold the result.
 /// If not, this returns null.
 static Value *SimplifyBinOp(unsigned Opcode, Value *LHS, Value *RHS,
                             const SimplifyQuery &Q, unsigned MaxRecurse) {
   switch (Opcode) {
   case Instruction::Add:
     return SimplifyAddInst(LHS, RHS, false, false, Q, MaxRecurse);
   case Instruction::Sub:
Value *llvm::SimplifyInstruction(Instruction *I, const SimplifyQuery &SQ,
                                  OptimizationRemarkEmitter *ORE) {
   const SimplifyQuery Q = SQ.CxtI ? SQ : SQ.getWithInstruction(I);
   Value *Result;
 
   switch (I->getOpcode()) {
   default:
     Result = ConstantFoldInstruction(I, Q.DL, Q.TLI);
     break;
+  case Instruction::FNeg:
+    Result = SimplifyFNegInst(I->getOperand(0), I->getFastMathFlags(), Q);
+    break;
   case Instruction::FAdd:
     Result = SimplifyFAddInst(I->getOperand(0), I->getOperand(1),
                               I->getFastMathFlags(), Q);
     break;
   case Instruction::Add:
     Result =
         SimplifyAddInst(I->getOperand(0), I->getOperand(1),
                         Q.IIQ.hasNoSignedWrap(cast<BinaryOperator>(I)),

llvm/test/Analysis/ConstantFolding/fneg.ll

 ; NOTE: Assertions have been autogenerated by utils/update_test_checks.py
-; RUN: opt < %s -constprop -S | FileCheck %s
+; RUN: opt < %s -instsimplify -S | FileCheck %s
 
 define float @fneg_constant() {
 ; CHECK-LABEL: @fneg_constant(
 ; CHECK-NEXT:    ret float 0.000000e+00
 ;
   %r = fneg float -0.0
   ret float %r
 }
 
 define float @fneg_undef() {
 ; CHECK-LABEL: @fneg_undef(
 ; CHECK-NEXT:    ret float undef
 ;
   %r = fneg float undef
   ret float %r
 }
 
 define float @fneg_fneg_var(float %a) {
 ; CHECK-LABEL: @fneg_fneg_var(
-; CHECK-NEXT:    [[R:%.*]] = fneg float [[A:%.*]]
-; CHECK-NEXT:    [[R1:%.*]] = fneg float [[R]]
-; CHECK-NEXT:    ret float [[R1]]
+; CHECK-NEXT:    ret float [[A:%.*]]
 ;
   %r  = fneg float %a
   %r1 = fneg float %r
   ret float %r1
 }
 
 define <4 x float> @fneg_constant_elts_v4f32() {
 ; CHECK-LABEL: @fneg_constant_elts_v4f32(