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

Thu Jul 21 03:49:39 PDT 2022

dmgreen 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.
FMOVWHr requires FullFP16.  It could use FMOVWSr instead, but then the types are a little awkward.

The call to setOperationAction is currently inside a HasFullFP16 block, so It could be moved out and we could always be using generic instructions. If we do go that route then make sure that we have tests for it.

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

https://reviews.llvm.org/D129989