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

[Michael Kitzan via Phabricator via llvm-commits]

mkitzan 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>">;
----------------
paquette wrote:
> 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}); }])
> >;
> ```
> 
No reason, except I didn't see that we could have a list of opcodes. Will fix that.


================
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 <
----------------
paquette wrote:
> 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)
When they are all refactored into a single combine rule, we'll end up with one `matchinfo` for free.


================
Comment at: llvm/lib/CodeGen/GlobalISel/CombinerHelper.cpp:1449
+  default:
+    return None;
+  case TargetOpcode::G_FNEG: {
----------------
paquette wrote:
> This should probably assert or be a `llvm_unreachable`, since we never expect to run this with any other opcode.
Makes sense to have an `assert`. If somehow control flow ended up there, then the developer would likely want the compiler to `assert` rather than `return None`.


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

https://reviews.llvm.org/D86393