This is an archive of the discontinued LLVM Phabricator instance.

Support FNeg ConstantExpr folding
AbandonedPublic

Authored by cameron.mcinally on May 1 2019, 7:34 PM.

Details

Summary

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

Diff Detail

Repository
rL LLVM

Event Timeline

Herald added a project: Restricted Project. · View Herald TranscriptMay 1 2019, 7:34 PM
arsenm added inline comments.May 2 2019, 3:52 AM
llvm/lib/IR/ConstantFold.cpp
948–949

You can use neg(CV) and remove C1V

arsenm added inline comments.May 2 2019, 4:00 AM
llvm/test/CodeGen/X86/fp-fold.ll
226–242

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 marked an inline comment as done.

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

llvm/test/CodeGen/X86/fp-fold.ll
226–242

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 added inline comments.May 2 2019, 5:35 AM
llvm/test/CodeGen/X86/fp-fold.ll
226–242

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 added a comment.May 2 2019, 6:53 AM

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 marked an inline comment as done.May 2 2019, 7:53 AM
cameron.mcinally added inline comments.
llvm/test/CodeGen/X86/fp-fold.ll
226–242

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 added inline comments.May 2 2019, 8:02 AM
llvm/test/CodeGen/X86/fp-fold.ll
226–242

llc should definitely not be needed.

I would expect -instsimplify to work.

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 marked an inline comment as done.May 2 2019, 8:13 AM
cameron.mcinally added inline comments.
llvm/test/CodeGen/X86/fp-fold.ll
226–242

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 marked an inline comment as done.May 2 2019, 9:29 AM
cameron.mcinally added inline comments.
llvm/test/CodeGen/X86/fp-fold.ll
226–242

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...

Couple of updates...

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?

Couple of updates...

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 retitled this revision from Support FNeg constant folding to Support FNeg ConstantExpr folding.

Remove DAG changes as @spatel suggested.

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...

Add support for FNeg constant folding in InstructionSimplify.

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

arsenm added inline comments.May 3 2019, 6:05 AM
llvm/test/CodeGen/X86/vec_fneg.ll
144

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

cameron.mcinally marked an inline comment as done.May 3 2019, 6:35 AM
cameron.mcinally added inline comments.
llvm/test/CodeGen/X86/vec_fneg.ll
144

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 added inline comments.May 3 2019, 6:39 AM
llvm/test/CodeGen/X86/vec_fneg.ll
144

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 marked an inline comment as done.May 3 2019, 6:47 AM
cameron.mcinally added inline comments.
llvm/test/CodeGen/X86/vec_fneg.ll
144

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 added a comment.May 3 2019, 7:41 AM

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 marked an inline comment as done.May 3 2019, 7:43 AM
cameron.mcinally added inline comments.
llvm/test/CodeGen/X86/fp-fold.ll
226–242

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.

Remove misplaced test/CodeGen tests.

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

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...

This comment was removed by cameron.mcinally.
cameron.mcinally abandoned this revision.May 5 2019, 12:23 PM

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