[PATCH] D26802: [X86][AVX512] Detect repeated constant patterns in BUILD_VECTOR suitable for broadcasting.

[Sanjay Patel via Phabricator via llvm-commits]

spatel accepted this revision.
spatel added a comment.
This revision is now accepted and ready to land.

LGTM - see inline comments for some nits.



================
Comment at: lib/Target/X86/X86ISelLowering.cpp:6336
+
+static bool isUseOfShuffleThroughBitcast(SDNode *N) {
+  for (SDNode::use_iterator I = N->use_begin(), E = N->use_end(); I != E; ++I) {
----------------
The function name confused me because the bitcast is optional, right?
Would it make more sense to just call this 'isUseOfShuffle()'?


================
Comment at: lib/Target/X86/X86ISelLowering.cpp:6337
+static bool isUseOfShuffleThroughBitcast(SDNode *N) {
+  for (SDNode::use_iterator I = N->use_begin(), E = N->use_end(); I != E; ++I) {
+    if (isTargetShuffle((*I)->getOpcode()))
----------------
Use range-for-loop?
  for (auto *U : N->uses()) 



================
Comment at: lib/Target/X86/X86ISelLowering.cpp:6383
+        SplatBitSize < VT.getSizeInBits()) {
+      // Avoid replacing with broadcast when its an use of a shuffle
+      // instruction to preserve the present custom lowering of shuffles.
----------------
its an -> it's a


================
Comment at: lib/Target/X86/X86ISelLowering.cpp:6389
+      const TargetLowering &TLI = DAG.getTargetLoweringInfo();
+      LLVMContext *Con = DAG.getContext();
+      MVT PVT = TLI.getPointerTy(DAG.getDataLayout());
----------------
'LLVMContext' is more commonly abbreviated as 'Ctx' to make it more distinguishable from abbreviations of 'Constant'.


================
Comment at: lib/Target/X86/X86ISelLowering.cpp:6413-6414
+          // Load the constant and broadcast it.
+          // AVX have support for 32 and 64 bit broadcast for floats only.
+          // No 64bit integer in 32bit subtarget.
+          MVT CVT = MVT::getFloatingPointVT(SplatBitSize);
----------------
I'd hoist these last 2 lines of the comment above to where you check if Subtarget.hasAVX().


https://reviews.llvm.org/D26802