Follow-up of rL371321 that added some more FP16 patterns, and an attempt to
reduce the copy-pasting and make this slightly more readable.
Details
Diff Detail
Event Timeline
I like the patch. Thanks!
llvm/lib/Target/AArch64/AArch64InstrInfo.cpp | ||
---|---|---|
3669 | What about avoiding the cond_expr with an or_expr? Something like this: Found = Match(1); Found |= Match(2); |
llvm/lib/Target/AArch64/AArch64InstrInfo.cpp | ||
---|---|---|
3669 | Yeah, I like that, that's even better. Thanks for reviewing, I will make this change before committing. |
Thanks for reviewing, I will make this change before committing
Ah, sorry, I thought I saw a LGTM. I will upload a new diff first.
Never mind. You cannot do the other ones as it would call Match too many times and would not follow the semantics of the original code.
The patch looks good to me as is.
llvm/lib/Target/AArch64/AArch64InstrInfo.cpp | ||
---|---|---|
3680 | 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. |
llvm/lib/Target/AArch64/AArch64InstrInfo.cpp | ||
---|---|---|
3680 | 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. |
llvm/lib/Target/AArch64/AArch64InstrInfo.cpp | ||
---|---|---|
3680 | 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. |
What about avoiding the cond_expr with an or_expr? Something like this: