[PATCH] D86393: [GISel] Add combines for unary FP instrs with constant operand

[Jessica Paquette via Phabricator via llvm-commits]

paquette added inline comments.


================
Comment at: llvm/include/llvm/Target/GlobalISel/Combine.td:294
 
+// Fold fneg(cst) to cst result of fneg operation
+def constant_fneg_matchinfo: GIDefMatchData<"Optional<APFloat>">;
----------------
Is there any reason these are all separate combines, when they're all using the same function? Have you found it useful to be able to turn these on/off per-opcode?

Most other combines look like this:

```
def fconstant_matchinfo: GIDefMatchData<"Optional<APFloat>">;
def constant_fold_unary: GICombineRule <
  (defs root:$root, fconstant_matchinfo:$info),
  (match (wip_match_opcode G_FNEG, G_FABS, G_FPTRUNC, G_FSQRT, G_FLOG2):$root,
    [{ return Helper.matchCombineConstantFoldFpUnary(*${root}, ${info}); }]),
  (apply [{ return Helper.applyCombineConstantFoldFpUnary(*${root}, ${info}); }])
>;
```



================
Comment at: llvm/include/llvm/Target/GlobalISel/Combine.td:295
+// Fold fneg(cst) to cst result of fneg operation
+def constant_fneg_matchinfo: GIDefMatchData<"Optional<APFloat>">;
+def constant_fneg: GICombineRule <
----------------
Do you need separate `matchinfo` definitions?

I've noticed all the combines do this, but I think it would be better to just say

```
def fconstant_matchinfo: GIDefMatchData<"Optional<APFloat>">;
```

and then reuse it in every combine that uses it versus redefining matchinfo for every combine. (The rest of the combines could probably be cleaned up similarly in a later commit if this works)


================
Comment at: llvm/lib/CodeGen/GlobalISel/CombinerHelper.cpp:1449
+  default:
+    return None;
+  case TargetOpcode::G_FNEG: {
----------------
This should probably assert or be a `llvm_unreachable`, since we never expect to run this with any other opcode.


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

https://reviews.llvm.org/D86393