[PATCH] D133433: [AArch64]: Force generating code compatible to streaming mode

[Sander de Smalen via Phabricator via llvm-commits]

sdesmalen added a comment.

Thanks @hassnaa-arm! I just left a few more nits mostly around the names, but other than that it looks really good!



================
Comment at: llvm/lib/Target/AArch64/AArch64ISelLowering.cpp:12027
 
+  // try overriding NEON if possible.
   if (useSVEForFixedLengthVectorVT(VT))
----------------
nit: comment can be removed?


================
Comment at: llvm/lib/Target/AArch64/AArch64ISelLowering.cpp:12391
   case ISD::SHL:
+    // override NEON if possible.
     if (VT.isScalableVector() || useSVEForFixedLengthVectorVT(VT))
----------------
nit: comment can be removed?


================
Comment at: llvm/lib/Target/AArch64/AArch64ISelLowering.cpp:12404
   case ISD::SRL:
+    // override NEON if possible.
     if (VT.isScalableVector() || useSVEForFixedLengthVectorVT(VT)) {
----------------
nit: comment can be removed?


================
Comment at: llvm/lib/Target/AArch64/AArch64Subtarget.cpp:69
+static cl::opt<bool>
+    ForceStreamingModeCompatibleSVE("force-streaming-mode-compatible-sve",
+                                 cl::init(false), cl::Hidden);
----------------
nit: I think that 'mode' is kind of implied from 'streaming compatible', so you remove `-mode` from the name, i.e. `force-streaming-mode-compatible-sve -> force-streaming-compatible-sve`.
Same request for the name of the variable, i.e. `ForceStreamingModeCompatibleSVE -> ForceStreamingCompatibleSVE`


================
Comment at: llvm/lib/Target/AArch64/AArch64Subtarget.cpp:441
+  if (ForceStreamingModeCompatibleSVE)
+    return hasSVE();
+
----------------
Should this return `true` always and instead have `assert(hasSVE() && "Expected SVE to be available")` ?
If someone forces using streaming-compatible code, SVE //must// be available. (and given that its not a user-exposed feature in Clang, it's fine for the compiler to crash if someone would use this feature while forgetting to set `+sve` somehow)


================
Comment at: llvm/lib/Target/AArch64/AArch64Subtarget.cpp:447
+
+bool AArch64Subtarget::forceStreamingModeCompatibleSVE() const {
+  if (ForceStreamingModeCompatibleSVE)
----------------
nit: `forceStreamingCompatibleSVE` (see other comment above)


================
Comment at: llvm/lib/Target/AArch64/AArch64Subtarget.cpp:69
+static cl::opt<bool>
+    ForceSVEWhenStreamingCompatible("force-sve-when-streaming-compatible",
+                                    cl::init(false), cl::Hidden);
----------------
hassnaa-arm wrote:
> sdesmalen wrote:
> > Can you rename this variable to `ForceStreamingCompatibleSVE`? (likewise change the name of the flag to `-force-streaming-compatible-sve`)
> > 
> > The current name `ForceSVEWhenStreamingCompatible` suggests to use the full range of SVE instructions when in streaming-compatible mode, even the instructions that would be illegal in that mode, but that would be incorrect.
> so there are some SVE instruction that are illegal in streaming mode ? like what ?
> because I was checking only for NEON illegal instructions, not SVE illegal instruction.
Streaming SVE is a subset of SVE, in that some SVE instructions (e.g. gather/scatter) are not valid in Streaming Mode.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D133433