[PATCH] D114946: [AArch64] Add instruction selection for strict FP

[Dave Green via Phabricator via llvm-commits]

dmgreen added a comment.

This is still a pretty big patch. Is it possible to break it up into some logically separate parts?



================
Comment at: llvm/lib/Target/AArch64/AArch64ISelLowering.cpp:1424
+
+  // Strict operations that correspond to a single instruction are legal, though
+  // for fp16 they need to be promoted/expanded when we don't have those
----------------
Is it better to have a section like this which is all the "Strict-fp handling", or should all this code be next to the existing operations?

We already have ISD::STRICT_FP_TO_SINT above next to ISD::FP_TO_SINT. And if SVE was added I would expect it to be in the SVE section (not that this is super well laid out). Should the ISD::STRICT_FCOS be next to the ISD::FCOS?


================
Comment at: llvm/test/CodeGen/AArch64/fp-intrinsics.ll:1
-; RUN: llc -mtriple=aarch64-none-eabi %s -o - | FileCheck %s
+; RUN: llc -mtriple=aarch64-none-eabi %s -o - | FileCheck %s --check-prefixes=CHECK,CHECK-NOFP16
+; RUN: llc -mtriple=aarch64-none-eabi -mattr=+fullfp16 %s -o - | FileCheck %s --check-prefixes=CHECK,CHECK-FP16
----------------
john.brawn wrote:
> dmgreen wrote:
> > Using update_llc_test_checks would be good, I think.
> It doesn't do a good job of handling the slight differences between the non-global-isel and global-isel output. It generates check lines that work for one but fail with the other.
That might be because the order of the check prefixes matter when it generates the checks, from most-general to least general. This file is bound to get updated by someone at some point to run the scripts, it's the only way to keep files like this maintainable, we might as well do it now.

The file is very big though, and looks like it would be better as a few different test files.


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

https://reviews.llvm.org/D114946