[PATCH] D124325: [AArch64][SVE] Support logical operation BIC with DestructiveBinary patterns

[Allen zhong via Phabricator via llvm-commits]

Allen marked 5 inline comments as done.
Allen added inline comments.


================
Comment at: llvm/lib/Target/AArch64/AArch64ExpandPseudoInsts.cpp:592
+  // Zeros the lanes in z0 that aren't active in p0 with sequence movprfx
+  // z0.b, p0/z, z0.b; add z0.b, z0.b, #0;
+  if (DType == AArch64::DestructiveBinary &&
----------------
paulwalker-arm wrote:
> Allen wrote:
> > paulwalker-arm wrote:
> > > efriedma wrote:
> > > > paulwalker-arm wrote:
> > > > > paulwalker-arm wrote:
> > > > > > Do any of the tests exercise this code?
> > > > > I don't believe this is safe because only predicated instructions are allowed to follow a predicated `movprfx` instruction. There's a section within
> > > > > ```
> > > > > https://developer.arm.com/documentation/ddi0487/latest/
> > > > > 
> > > > > Data processing - SVE
> > > > > -> Move operations
> > > > > --> Move prefix
> > > > > ```
> > > > > that details which instructions are allowed to follow a `movprfx`. 
> > > > Oh, oops, I misread the documentation.  Can we use `lsl z0.b, p0/m, z0.b, #0` instead?
> > > I think so but will check if there's an architecture preferred answer and will report back.
> > Thanks @paulwalker-arm and @efriedma.
> > If we don't have a better architecture preferred answer, how about using **lsl z0.b, p0/m, z0.b, #0** first? In fact, there are very few scenarios where this additional instruction is required, such as case **bic_i64_zero_no_comm**.
> lsl is not a great fit for current implementations but as you say this will only be used in rare cases that we ultimately want to solve during register allocation anyway. Plus, looking over the instruction set I'm not sure there's a non-shift alternative so yes let's go with `lsl z0.b, p0/m, z0.b, #0`.
Done, thanks for your suggestion.


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

https://reviews.llvm.org/D124325