[PATCH] D118979: [AArch64] Set maximum VF with shouldMaximizeVectorBandwidth

[JinGu Kang via Phabricator via llvm-commits]

jaykang10 added inline comments.


================
Comment at: llvm/lib/Target/AArch64/AArch64TargetTransformInfo.cpp:39
 
+static cl::opt<bool> AArch64MaximizeBandwidth(
+    "aarch64-vectorizer-maximize-bandwidth", cl::init(true), cl::Hidden,
----------------
paulwalker-arm wrote:
> dmgreen wrote:
> > There is a vectorizer-maximize-bandwidth option is the vectorizer that can override the target option for shouldMaximizeVectorBandwidth. I don't think adding an aarch64 option is necessary, can you remove it?
> I agree.  My original ask was because I thought there were concerns about enabling this by default.  Given the flag still defaults to on and it seems we're happy to make this change for AArch64 I retract my previous ask.
Yep, let me remove this option.


================
Comment at: llvm/lib/Transforms/Vectorize/LoopVectorize.cpp:5589
 
+  TargetTransformInfo::RegisterKind K =
+      ComputeScalableMaxVF ? TargetTransformInfo::RGK_ScalableVector
----------------
dmgreen wrote:
> K is a bit of a short name. Perhaps use RegKind or something like it?
Yep, let me change it.


================
Comment at: llvm/test/Transforms/LoopVectorize/AArch64/sve-illegal-type.ll:90
 ; CHECK: store i1 %[[EXTRACT1]], i1* %dst
-; CHECK: %[[EXTRACT2:.*]] = extractelement <2 x i1> %[[ICMP]], i32 1
+; CHECK: %[[EXTRACT2:.*]] = extractelement <64 x i1> %[[ICMP]], i32 1
 ; CHECK: store i1 %[[EXTRACT2]], i1* %dst
----------------
dmgreen wrote:
> This is worrying - should it be vectorizing 64x for in i1 type! (and are there a lot of other extracts now)?
When I checked it, it looked the dagcombiner combines the 64 times i1 extract_vector_elt and store nodes to one 64 bit store node.
Let me check it again.


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

https://reviews.llvm.org/D118979