[PATCH] D61419: Support FNeg ConstantExpr folding

[Cameron McInally via Phabricator via llvm-commits]

cameron.mcinally marked an inline comment as done.
cameron.mcinally added inline comments.


================
Comment at: llvm/test/CodeGen/X86/fp-fold.ll:226-242
+
+define float @fneg_undef() {
+; ANY-LABEL: fneg_undef:
+; ANY:       # %bb.0:
+; ANY-NEXT:    retq
+  %r = fneg float undef
+  ret float %r
----------------
cameron.mcinally wrote:
> cameron.mcinally wrote:
> > arsenm wrote:
> > > cameron.mcinally wrote:
> > > > arsenm wrote:
> > > > > cameron.mcinally wrote:
> > > > > > arsenm wrote:
> > > > > > > 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?
> > > > > > > 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.
> > > > > 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
> > > > 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...
> > > llc should definitely not be needed.
> > > 
> > > I would expect -instsimplify to work.
> > > 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...
> > 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...
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`.


Repository:
  rL LLVM

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

https://reviews.llvm.org/D61419