[PATCH] D43306: [X86] Add pass to infer required-vector-width attribute based on size of function arguments and use of intrinsics

[Chandler Carruth via Phabricator via llvm-commits]

chandlerc added a comment.

Given the direction this is going, I'd suggest maybe renaming the attribute. I'm thinking something like `legal_vetor_width` or otherwise framing this as fundamentally a legalization constraint rather than anything more fundamental. I would also really suggest doing more than just one width shift in this patch because that will help ensure that the mechanism is working in the general way as intended.

I have a bunch of other nit-picky comments but can save those until this is more baked and you're looking to polish.

One last somewhat longer-term question is: have you thought about making this more targeted? Specifically, rather than making it a *type* legalization parameter, how crazy would it be to make this only apply to specific operations? I'm interested in potentially only preferring narrower vector widths for non-mov instructions so that loading and storing wider types can continue to occur. Thoughts?



================
Comment at: lib/Target/X86/X86VectorWidthInfer.cpp:58-62
+  // If the target doesn't support 512-bit vectors or doesn't prefer them,
+  // then there is nothing to do.
+  // TODO: Support this for 256 vs 128 as well?
+  if (!ST->hasAVX512() || ST->canExtendTo512DQ())
+    return false;
----------------
I think if you just delete this code, everything below already works for arbitrary vector widths?


================
Comment at: lib/Target/X86/X86VectorWidthInfer.cpp:116-117
+
+  // Remove and replace function's prefer-vector-width attribute.
+  RequiredWidth = PowerOf2Ceil(RequiredWidth);
+  F.removeFnAttr("prefer-vector-width");
----------------
You should be able to bound this (in both directions) by looking at the subtarget's register sizes. Which would somewhat illustrate that this is just parameterizing *legalization*, not the underlying physical machine description.


https://reviews.llvm.org/D43306