[PATCH] D129989: [AArch64] Add f16 fpimm patterns

Thu Jul 21 03:48:42 PDT 2022

paulwalker-arm added inline comments.

================
Comment at: llvm/lib/Target/AArch64/AArch64InstrInfo.td:1629
+def : Pat<(f16 fpimm:$in),
+  (COPY_TO_REGCLASS (MOVi32imm (bitcast_fpimm_to_i32 f16:$in)), FPR16)>;
 def : Pat<(f32 fpimm:$in),
----------------
paulwalker-arm wrote:
> dmgreen wrote:
> > COPY's are quite loose in their definition, but it is probably best to keep them copying between values of the same size. I think this can use FMOVWHr directly, so long as the instruction has already been defined.
> > It should probably have a: `let Predicates = [HasFullFP16]` too.
> Is `HasFullFP16` relevant? The lowering requires no special instructions so I'd even argue the `setOperationAction` call requires no protection.  What's important is whether the `f16` type is legal.  Once we get to operation lowering and isel we only need to protect instructions not types.
> 
> We made this mistake for `bf16` for SVE and it just lead to unnecessary complexity which was eventually removed.
I've checked and `FMOVWHr` does require `HasFullFP16` so the question becomes, should we allow good code generation even for the case when `HasFullFP16` is not available? Or is  `HasFullFP16` so universally available these days for us not to care?

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

https://reviews.llvm.org/D129989