[PATCH] D79357: [SveEmitter] Add builtins for svdup and svindex

[Sander de Smalen via Phabricator via cfe-commits]

sdesmalen marked 2 inline comments as done.
sdesmalen added inline comments.


================
Comment at: clang/include/clang/Basic/arm_sve.td:1050
 def SVCOMPACT    : SInst<"svcompact[_{d}]",   "dPd",  "ilUiUlfd",        MergeNone, "aarch64_sve_compact">;
-//  SVDUP_LANE    (to land in D78750)
+def SVDUP_LANE   : SInst<"svdup_lane[_{d}]",  "ddL",  "csilUcUsUiUlhfd", MergeNone, "aarch64_sve_tbl">; // Implemented using tbl
 def SVDUPQ_LANE  : SInst<"svdupq_lane[_{d}]", "ddn",  "csilUcUsUiUlhfd", MergeNone, "aarch64_sve_dupq_lane">;
----------------
efriedma wrote:
> "Implemented using tbl" isn't really a helpful comment.
> 
> I guess you're doing it this way so the lane doesn't have to be a constant?  (And then I guess you can optimize it in the backend.)
Yes that's right. I can probably clarify the comment a bit more.


================
Comment at: clang/lib/CodeGen/CGBuiltin.cpp:8047
+    Value *PFalse = Constant::getNullValue(PTrue->getType());
+    Value *Sel = Builder.CreateSelect(CmpNE, PTrue, PFalse);
+    return EmitSVEPredicateCast(Sel, cast<llvm::ScalableVectorType>(Ty));
----------------
efriedma wrote:
> The select might not be the best approach. I mean, it's semantically correct, but the backend currently doesn't know how to lower a select like that.  And even if it did, it's probably better to avoid generating it for now.
> 
> Maybe this should use whilelo directly, since that's what the backend would eventually use anyway.
> but the backend currently doesn't know how to lower a select like that
Yes I noticed, that was the reason for D79356 (added as parent revision)

> And even if it did, it's probably better to avoid generating it for now.
Is that because there are common optimisations on the select? I thought it would be safe to use the 'full vector' form of `select` where the predicate is a scalar (and both operands are splats)


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

https://reviews.llvm.org/D79357