[PATCH] D156109: [AArch64][SME] Create new interface for isSVEAvailable.

[Paul Walker via Phabricator via llvm-commits]

paulwalker-arm added inline comments.


================
Comment at: llvm/lib/Target/AArch64/AArch64ISelLowering.cpp:19564-19566
+    if (Subtarget->isSVEAvailable())
+      return combineSVEReductionOrderedFP(N, AArch64ISD::FADDA_PRED, DAG);
+    break;
----------------
As discussed this shouldn't be necessary because we require users of the target specific SVE intrinsics to know what they're doing.   There are several cases where incorrect usage of these intrinsics can lead to unexpected behaviour and we make no attempt to guard against them.


================
Comment at: llvm/lib/Target/AArch64/AArch64Subtarget.h:221
+  /// in streaming-SVE mode or when the target has FEAT_FA64 enabled.
+  bool isFullSVEAvailable() const;
+
----------------
sdesmalen wrote:
> paulwalker-arm wrote:
> > I don't like this name.  FullSVE isn't really a thing, you either have SVE or you don't, much like with NEON.  Can this just be `isSVEAvailable()`?
> I'm happy to change it to `isSVEAvailable()`, but it's perhaps a bit confusing in the context of other interfaces such as `useSVEforFixedLengthVectors`, where `SVE` has a meaning where it allows for the streaming-compatible subset of SVE, whereas for `isSVEAvailable` it strictly relates to the full set of SVE instructions.
For clarity I interpret `useSVEforFixedLengthVectors` as just "use SVE instructions for fixed length vectors".  It makes no judgement whether such instructions are streaming-compatible, it's just that most are and the few that are not use different code paths based on a more specific query like isSVEAvailable or isStreamingCompatible after this top level decision.

That's not to say the `useSVEforFixedLengthVectors` interface is perfect when considering this newer requirement but for now it seems to be working ok.


================
Comment at: llvm/test/CodeGen/AArch64/sve-fp-reduce-fadda.ll:4-5
 
-; FIXME: Streaming-compatible SVE doesn't include FADDA, so this shouldn't compile!
-; RUN: llc -mattr=+sve -force-streaming-compatible-sve < %s | FileCheck %s
+; Streaming-compatible SVE doesn't include FADDA, so this shouldn't compile!
+; RUN: not --crash llc -mattr=+sve -force-streaming-compatible-sve < %s
 
----------------
sdesmalen wrote:
> paulwalker-arm wrote:
> > I'm not sure it worth having RUN lines that we know will crash?  If we care about ensuring such IR isn't code generated then we should update the IR Verifier to generate a clean failure rather than require a compiler crash.
> I'd rather keep the RUN line to make sure the code I added in the patch is protected by a test and therefore cannot be removed without breaking something.
Fair enough.


================
Comment at: llvm/test/CodeGen/AArch64/sve-intrinsics-fp-reduce-fadda.ll:4-5
 
-; FIXME: Streaming-compatible SVE doesn't include FADDA, so this shouldn't compile!
-; RUN: llc -mtriple=aarch64--linux-gnu -mattr=+sve -force-streaming-compatible-sve < %s | FileCheck %s
+; Streaming-compatible SVE doesn't include FADDA, so this shouldn't compile!
+; RUN: not --crash llc -mtriple=aarch64--linux-gnu -mattr=+sve -force-streaming-compatible-sve < %s
 
----------------
As discuss whilst protecting the common intrinsic has value the same is not true for the target specific SVE intrinsics.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D156109