[PATCH] D114542: [CodeGen][SVE] Use whilelo instruction when lowering @llvm.get.active.lane.mask

[Sjoerd Meijer via Phabricator via llvm-commits]

SjoerdMeijer added a comment.

Just wanted to check how this works for SVE. 
For MVE, we have to work quite hard to lower this to MVE specific intrinsic. In MVETailPredication.cpp we have a function `IsSafeActiveMask` that performs the following checks:

  // The active lane intrinsic has this form:
  //
  //    @llvm.get.active.lane.mask(IV, TC)
  //
  // Here we perform checks that this intrinsic behaves as expected,
  // which means:
  //
  // 1) Check that the TripCount (TC) belongs to this loop (originally).
  // 2) The element count (TC) needs to be sufficiently large that the decrement
  //    of element counter doesn't overflow, which means that we need to prove:
  //        ceil(ElementCount / VectorWidth) >= TripCount
  //    by rounding up ElementCount up:
  //        ((ElementCount + (VectorWidth - 1)) / VectorWidth
  //    and evaluate if expression isKnownNonNegative:
  //        (((ElementCount + (VectorWidth - 1)) / VectorWidth) - TripCount
  // 3) The IV must be an induction phi with an increment equal to the
  //    vector width.

All these checks are performed because we transform the loop into a different form (a tail-predicated one ) and new expressions are introduced that should not overflow. The rationale was that it wouldn't be difficult to imagine that a user-facing intrinsic will be introduced one day, in which we need these checks to see if things behave as expected (@efriedma can correct me if I'm wrong here). Please note that currently this intrinsic is emitted only by the loop vectoriser, and these issues should not occur.

Now the question is, we don't perform any of these checks, but do we need them?
The sanity check that the IV belongs to a loop would be nice, but that seems problematic here because in SelectionDAG we don't have the Loop view. 
The overflow check that we do for MVE and the element count expression and decrement that we introduce does not seem to be applicable for SVE, is that right? That would be good news because that was the hardest part for MVE.



================
Comment at: llvm/include/llvm/CodeGen/TargetLowering.h:430
+  /// using generic code in SelectionDAGBuilder, or should remain as an
+  /// intrinsic. For certain targets it's beneficial to leave as an intrinsic
+  /// due to a one-to-one mapping between the intrinsic and an available target
----------------
> ... or should remain as an intrinsic.

Perhaps clarify that intrinsic is a target specific intrinsic in that case.


================
Comment at: llvm/lib/Target/AArch64/AArch64ISelLowering.cpp:1510
+  if (!Subtarget->hasSVE())
+    return true;
+
----------------
I was expecting `return false` here, now I am a bit confused...


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D114542