fixup! respond to some more reviews

michaelmaitland · michaelmaitland · commit cf71fdded62e · 2024-09-03T11:18:29.000-07:00
diff --git a/llvm/include/llvm/Analysis/CSADescriptors.h b/llvm/include/llvm/Analysis/CSADescriptors.h
@@ -1,4 +1,4 @@
-//===- llvm/Analysis/CSADescriptors.h - CSA Descriptors --*- C++ -*-===//
+//===------------- CSADescriptors.h - CSA Descriptors -----------*- C++ -*-===//
 //
 // Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
 // See https://llvm.org/LICENSE.txt for license information.
diff --git a/llvm/lib/Analysis/CSADescriptors.cpp b/llvm/lib/Analysis/CSADescriptors.cpp
@@ -1,4 +1,4 @@
-//=== llvm/Analysis/CSADescriptors.cpp - CSA Descriptors -*- C++ -*-===//
+//===----------- CSADescriptors..cpp - CSA Descriptors ----------*- C++ -*-===//
 //
 // Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
 // See https://llvm.org/LICENSE.txt for license information.
@@ -19,43 +19,43 @@ using namespace llvm::PatternMatch;
 
 #define DEBUG_TYPE "csa-descriptors"
 
+/// Return CSADescriptor that describes a CSA that matches one of these
+/// patterns:
+///   phi loop_inv, (select cmp, value, phi)
+///   phi loop_inv, (select cmp, phi, value)
+///   phi (select cmp, value, phi), loop_inv
+///   phi (select cmp, phi, value), loop_inv
+/// If the CSA does not match any of these paterns, return a CSADescriptor
+/// that describes an InvalidCSA.
 CSADescriptor CSADescriptor::isCSAPhi(PHINode *Phi, Loop *TheLoop) {
-  // Return CSADescriptor that describes a CSA that matches one of these
-  // patterns:
-  //   phi loop_inv, (select cmp, value, phi)
-  //   phi loop_inv, (select cmp, phi, value)
-  //   phi (select cmp, value, phi), loop_inv
-  //   phi (select cmp, phi, value), loop_inv
-  // If the CSA does not match any of these paterns, return a CSADescriptor
-  // that describes an InvalidCSA.
 
   // Must be a scalar
   Type *Type = Phi->getType();
   if (!Type->isIntegerTy() && !Type->isFloatingPointTy() &&
       !Type->isPointerTy())
-    return CSADescriptor();
+    return {};
 
   // Match phi loop_inv, (select cmp, value, phi)
   //    or phi loop_inv, (select cmp, phi, value)
   //    or phi (select cmp, value, phi), loop_inv
   //    or phi (select cmp, phi, value), loop_inv
   if (Phi->getNumIncomingValues() != 2)
-    return CSADescriptor();
-  auto SelectInstIt = find_if(Phi->incoming_values(), [&Phi](Use &U) {
+    return {};
+  auto SelectInstIt = find_if(Phi->incoming_values(), [&Phi](const Use &U) {
     return match(U.get(), m_Select(m_Value(), m_Specific(Phi), m_Value())) ||
            match(U.get(), m_Select(m_Value(), m_Value(), m_Specific(Phi)));
   });
   if (SelectInstIt == Phi->incoming_values().end())
-    return CSADescriptor();
+    return {};
   auto LoopInvIt = find_if(Phi->incoming_values(), [&](Use &U) {
     return U.get() != *SelectInstIt && TheLoop->isLoopInvariant(U.get());
   });
   if (LoopInvIt == Phi->incoming_values().end())
-    return CSADescriptor();
+    return {};
 
   // Phi or Sel must be used only outside the loop,
   // excluding if Phi use Sel or Sel use Phi
-  auto IsOnlyUsedOutsideLoop = [=](Value *V, Value *Ignore) {
+  auto IsOnlyUsedOutsideLoop = [&](Value *V, Value *Ignore) {
     return all_of(V->users(), [Ignore, TheLoop](User *U) {
       if (U == Ignore)
         return true;
@@ -64,10 +64,11 @@ CSADescriptor CSADescriptor::isCSAPhi(PHINode *Phi, Loop *TheLoop) {
       return true;
     });
   };
-  auto *Sel = cast<SelectInst>(SelectInstIt->get());
-  auto *LoopInv = LoopInvIt->get();
-  if (!IsOnlyUsedOutsideLoop(Phi, Sel) || !IsOnlyUsedOutsideLoop(Sel, Phi))
-    return CSADescriptor();
+  Instruction *Select = cast<SelectInst>(SelectInstIt->get());
+  Value *LoopInv = LoopInvIt->get();
+  if (!IsOnlyUsedOutsideLoop(Phi, Select) ||
+      !IsOnlyUsedOutsideLoop(Select, Phi))
+    return {};
 
-  return CSADescriptor(Phi, Sel, LoopInv);
+  return {Phi, Select, LoopInv};
 }
diff --git a/llvm/lib/Transforms/Vectorize/VPlan.h b/llvm/lib/Transforms/Vectorize/VPlan.h
@@ -3986,7 +3986,6 @@ class VPlanSlp {
   /// Return true if all visited instruction can be combined.
   bool isCompletelySLP() const { return CompletelySLP; }
 };
-
 } // end namespace llvm
 
 #endif // LLVM_TRANSFORMS_VECTORIZE_VPLAN_H
diff --git a/llvm/lib/Transforms/Vectorize/VPlanRecipes.cpp b/llvm/lib/Transforms/Vectorize/VPlanRecipes.cpp
@@ -679,8 +679,7 @@ Value *VPInstruction::generatePerPart(VPTransformState &State, unsigned Part) {
       State.set(this, InitMask, Part);
       return InitMask;
     }
-    Value *V = State.get(this, Part - 1);
-    return V;
+    return State.get(this, Part - 1);
   }
   case VPInstruction::CSAInitData: {
     if (Part == 0) {
@@ -689,8 +688,7 @@ Value *VPInstruction::generatePerPart(VPTransformState &State, unsigned Part) {
       State.set(this, InitData, Part);
       return InitData;
     }
-    Value *V = State.get(this, Part - 1);
-    return V;
+    return State.get(this, Part - 1);
   }
   case VPInstruction::CSAMaskPhi: {
     if (Part == 0) {
@@ -2318,7 +2316,7 @@ void VPCSAExtractScalarRecipe::execute(VPTransformState &State) {
     // inactive, which would also cause the reduction to have value 0.
     Value *MaybeLastIdx = State.Builder.CreateIntMaxReduce(ActiveLaneIdxs);
     Value *IsLaneZeroActive =
-        State.Builder.CreateExtractElement(MaskSel, (uint64_t)0);
+        State.Builder.CreateExtractElement(MaskSel, static_cast<uint64_t>(0));
     Value *Zero = ConstantInt::get(MaybeLastIdx->getType(), 0);
     Value *MaybeLastIdxEQZero = State.Builder.CreateICmpEQ(MaybeLastIdx, Zero);
     Value *And = State.Builder.CreateAnd(IsLaneZeroActive, MaybeLastIdxEQZero);
diff --git a/llvm/lib/Transforms/Vectorize/VPlanTransforms.cpp b/llvm/lib/Transforms/Vectorize/VPlanTransforms.cpp
@@ -1442,17 +1442,8 @@ bool VPlanTransforms::tryAddExplicitVectorLength(VPlan &Plan) {
 }
 
 void VPlanTransforms::addExplicitVectorLengthForCSA(
-    VPInstruction *VPEVL, const MapVector<PHINode *, VPCSAState *> &CSAStates,
-    bool PlanHasScalarVFOnly) {
-
-  // We build the scalar version of a CSA when VF=ElementCount::getFixed(1),
-  // which does not require an EVL.
-  if (PlanHasScalarVFOnly)
-    return;
-
-  for (auto CSA : CSAStates) {
-    VPCSAState *CSAState = CSA.second;
-
+    VPInstruction *VPEVL, const MapVector<PHINode *, VPCSAState *> &CSAStates) {
+  for (auto &[_, CSAState] : CSAStates) {
     // CSAAnyActive is used to keep track of whether any condition on the
     // current iteration is active. This is used to decide whether the mask
     // should be updated. When we are using EVL, we must only consider the first
diff --git a/llvm/lib/Transforms/Vectorize/VPlanTransforms.h b/llvm/lib/Transforms/Vectorize/VPlanTransforms.h
@@ -117,12 +117,10 @@ struct VPlanTransforms {
 
   /// Add recipes required to make CSA work with EVL based approach. This
   /// includes replacing \p CSAAnyActive with \p CSAAnyActiveEVL, and adding \p
-  /// CSAVLPhi and \p CSAVLSel instructions. No EVL specific recipes are added
-  /// when \p PlanHasScalarVFOnly since scalar version of a CSA does not require
-  /// an EVL.
+  /// CSAVLPhi and \p CSAVLSel instructions.
   static void addExplicitVectorLengthForCSA(
-      VPInstruction *VPEVL, const MapVector<PHINode *, VPCSAState *> &CSAStates,
-      bool PlanHasScalarVFOnly);
+      VPInstruction *VPEVL,
+      const MapVector<PHINode *, VPCSAState *> &CSAStates);
 };
 
 } // namespace llvm

Original file line number	Diff line number	Diff line change
`@@ -1,4 +1,4 @@`
`1`		`-//===- llvm/Analysis/CSADescriptors.h - CSA Descriptors --- C++ --===//`
	`1`	`+//===------------- CSADescriptors.h - CSA Descriptors ------------ C++ --===//`
`2`	`2`	`//`
`3`	`3`	`// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.`
`4`	`4`	`// See https://llvm.org/LICENSE.txt for license information.`