[PATCH] D67403: [AArch64] MachineCombiner FMA matching. NFC.

[Sjoerd Meijer via Phabricator via llvm-commits]

SjoerdMeijer marked an inline comment as done.
SjoerdMeijer added inline comments.


================
Comment at: llvm/lib/Target/AArch64/AArch64InstrInfo.cpp:3680
+                  ? true
+                  : Found;
+
----------------
sebpop wrote:
> sebpop wrote:
> > sebpop wrote:
> > > ... here and below, until the end of the patch, please replace cond_exprs with bit_or exprs.  Thanks!
> > What about instead of the cond_expr we write it as in the original code with if_stmts?
> > ```
> > if (Match(1))
> >   Found = true;
> > else if (Match(2))
> >   Found = true;
> > ```
> > I find this form more readable than the cond_expr.
> Or you could do
> ```
> Found |=
>         Match(AArch64::FMULSrr, 1, MCP::FMULADDS_OP1) ||
>         Match(AArch64::FMULv1i32_indexed, 1, MCP::FMLAv1i32_indexed_OP1);
> ```
> as the `||` will only evaluate the first operand when it results in `true` and avoid evaluating the second operand.
Yeah, I like that! I struggled with this patch quite a lot actually. I.e., I have tried different implementations to see what is most readable, because I just want to see as much straight line code as possible with just the opcodes that we are matching, and not any of the other clutter. So I have also tried the if-else that you suggested earlier, wasn't happy with that, it looked too much at the original code, but perhaps that is just a personal preference. I found the conditional expression to be the least distracting from what we are trying to do here. Your last suggestion is the best one, in my opinion, so will go for that one.


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

https://reviews.llvm.org/D67403