[TargetLowering] isVectorClearMaskLegal - use ArrayRef<int> instead of const SmallVectorImpl<int>&

[RKSimon]

Summary

This is more generic and matches isShuffleMaskLegal.

Under what circumstances would people want const SmallVectorImp<>& instead of ArrayRef<>?

[spatel]

Not sure when the ArrayRef would be insufficient, but it can always be changed back if that is actually necessary.

LGTM.

include/llvm/CodeGen/TargetLowering.h

751–753 ↗

(On Diff #152868)

Fix the comment typo since we're making a change - delete "This is used by".

lib/Target/X86/X86ISelLowering.h

996–998 ↗

(On Diff #152868)

Shouldn't need to repeat the documentation comment from the base class?

[lebedev.ri]

In D48591#1143450, @spatel wrote:

If it is modified, then SmallVectorImpl is needed, obviously.
Else
If not all users are passing something derived from SmallVectorImpl, then ArrayRef is better.
Else, if all users are derived from SmallVectorImpl, then it is best to pass that,
since it is just a downcast, while construction of ArrayRef is, well, construction.
Example: https://godbolt.org/g/zkYiC4

[spatel]

In D48591#1143466, @lebedev.ri wrote:

In D48591#1143450, @spatel wrote:

If it is modified, then SmallVectorImpl is needed, obviously.
Else
If not all users are passing something derived from SmallVectorImpl, then ArrayRef is better.
Else, if all users are derived from SmallVectorImpl, then it is best to pass that,
since it is just a downcast, while construction of ArrayRef is, well, construction.
Example: https://godbolt.org/g/zkYiC4

Ah - thanks for the example. I didn't consider the codegen difference (generality and consistency of the source code are the main motivations here I think, but good to actually know about the potential perf difference).

llvm/trunk/include/llvm/CodeGen/TargetLowering.h

public:
   }
 
   /// Returns true if the operation can trap for the value type.
   ///
   /// VT must be a legal type. By default, we optimistically assume most
   /// operations don't trap except for integer divide and remainder.
   virtual bool canOpTrap(unsigned Op, EVT VT) const;
 
-  /// Similar to isShuffleMaskLegal. This is used by Targets can use this to
-  /// indicate if there is a suitable VECTOR_SHUFFLE that can be used to replace
-  /// a VAND with a constant pool entry.
-  virtual bool isVectorClearMaskLegal(const SmallVectorImpl<int> &/*Mask*/,
+  /// Similar to isShuffleMaskLegal. Targets can use this to indicate if there
+  /// is a suitable VECTOR_SHUFFLE that can be used to replace a VAND with a
+  /// constant pool entry.
+  virtual bool isVectorClearMaskLegal(ArrayRef<int> /*Mask*/,
                                       EVT /*VT*/) const {
     return false;
   }
 
   /// Return how this operation should be treated: either it is legal, needs to
   /// be promoted to a larger size, needs to be expanded to some other code
   /// sequence, or the target has a custom expander for it.
   LegalizeAction getOperationAction(unsigned Op, EVT VT) const {

llvm/trunk/lib/Target/X86/X86ISelLowering.h

public:
     bool isFPImmLegal(const APFloat &Imm, EVT VT) const override;
 
     /// Targets can use this to indicate that they only support *some*
     /// VECTOR_SHUFFLE operations, those with specific masks. By default, if a
     /// target supports the VECTOR_SHUFFLE node, all mask values are assumed to
     /// be legal.
     bool isShuffleMaskLegal(ArrayRef<int> Mask, EVT VT) const override;
 
-    /// Similar to isShuffleMaskLegal. This is used by Targets can use this to
-    /// indicate if there is a suitable VECTOR_SHUFFLE that can be used to
-    /// replace a VAND with a constant pool entry.
-    bool isVectorClearMaskLegal(const SmallVectorImpl<int> &Mask,
-                                EVT VT) const override;
+    /// Similar to isShuffleMaskLegal. Targets can use this to indicate if there
+    /// is a suitable VECTOR_SHUFFLE that can be used to replace a VAND with a
+    /// constant pool entry.
+    bool isVectorClearMaskLegal(ArrayRef<int> Mask, EVT VT) const override;
 
     /// Returns true if lowering to a jump table is allowed.
     bool areJTsAllowed(const Function *Fn) const override;
 
     /// If true, then instruction selection should
     /// seek to shrink the FP constant of the specified type to a smaller type
     /// in order to save space and / or reduce runtime.
     bool ShouldShrinkFPConstant(EVT VT) const override {

llvm/trunk/lib/Target/X86/X86ISelLowering.cpp

bool X86TargetLowering::isShuffleMaskLegal(ArrayRef<int> M, EVT VT) const {
   if (VT.getSimpleVT().getSizeInBits() == 64)
     return false;
 
   // We only care that the types being shuffled are legal. The lowering can
   // handle any possible shuffle mask that results.
   return isTypeLegal(VT.getSimpleVT());
 }
 
-bool
-X86TargetLowering::isVectorClearMaskLegal(const SmallVectorImpl<int> &Mask,
+bool X86TargetLowering::isVectorClearMaskLegal(ArrayRef<int> Mask,
                                                EVT VT) const {
   // Don't convert an 'and' into a shuffle that we don't directly support.
   // vpblendw and vpshufb for 256-bit vectors are not available on AVX1.
   if (!Subtarget.hasAVX2())
     if (VT == MVT::v32i8 || VT == MVT::v16i16)
       return false;
 
   // Just delegate to the generic legality, clear masks aren't special.
   return isShuffleMaskLegal(Mask, VT);