[VPlan] Use underlying value for printing, if available.

[fhahn]

Summary

When the an underlying value is available, we can use its name for
printing, as discussed in D73078.

[Ayal]

llvm/lib/Transforms/Vectorize/VPlan.cpp

589

If now every VPValue that wraps a Value clearly says so in its name, does it hopefully make this "%vpA := %irX" part of "where:" redundant?

819

Even if UV does not have a name, could we still indicate that VPValue wraps it; possibly by calling UV->printAsOperand()?

[fhahn]

Updated to use Value::printAsOperand for all VPValues with underlying values.

The new format uses '%vp.ir.<underlying value name>' for non-constant underlying values. It seems we have to do a bit of extra work to drop the % from the printed name. getName() would just return the string, but we also want to include implicitly numbered IR values.

For constant values, it prints 'ir constant <constant value>'. Not sure if calling out that it's an IR constant is required though.

It also adds a hasResult() method to VPInstruction, because printAsOperand does not make sense on IR values of type void. The method is subsequently used to avoid printing a result name for VPInstruction with underlying values of void ty.

[fhahn]

llvm/lib/Transforms/Vectorize/VPlan.cpp

589

I can drop that in a follow-up, if that's ok?

819

Done! It required to add special handling for constants and also a bit of code to drop the % printed by printAsOperand, but now all underlying IR values should be printed consistently. A full list of the required changes can be found in the update comment.

[Ayal]

llvm/lib/Transforms/Vectorize/VPlan.cpp

409

Better call printAsOperand(O, SlotTracker) only when needed, i.e.,

if (hasResult()) {
  printAsOperand(O, SlotTracker);
  O << " = ";
}

589

Sure, by all means.

819

Thanks!
Could this be simplified, by always printing "ir<X>" where X is whatever printAsOperand provides, which could be ir<%tmp>, ir<%5>, ir<7>, ir<void>, letting the caller worry about not printing void?

And for VPValues w/o ingredients, print "vp<%x>" where x is the number assigned by SlotTracker? Currently this should be only BackedgeTakenCount, which ought to be assigned a meaningful name...

892

Suffice to check now instead if (!UV)

llvm/lib/Transforms/Vectorize/VPlan.h

739

Better look at the VPInstruction's Opcode to determine if it has a result?

1479

Better record the underlying ingredient to be used when printing, by doing new VPValue(V) instead.

[fhahn]

Address Ayal's comments, thanks!

[fhahn]

llvm/lib/Transforms/Vectorize/VPlan.cpp

819

Yes that format simplifies things. I think it looks OK as well.

892

If V is a VPInstruction, we should also only number instructions with results. Should be fixed

llvm/lib/Transforms/Vectorize/VPlan.h

739

I wanted to avoid to have such a check here initially, but it does not look too bad. Unless I missed an instruction, I think calls should be the only instruction where the opcode does not determine if it has a result or not. But just assuming calls have results should be fine.

Plenty of instructions without result are not likely to be VPInstructions, but I guess it does not hurt to include them here.

1479

Also better as a follow up?

[Ayal]

This looks good to me, thanks!
Added some minor final comments.

llvm/lib/Transforms/Vectorize/VPlan.cpp

893

Right. May be easier to read if reversed into an early-exiting

if (UV || (VPI && !VPI->hasResult()))
  return;

At some point it would be good to name VPValues (w/o underlying Values). These would need SlotTracker's attention, to break name reuses.

llvm/lib/Transforms/Vectorize/VPlan.h

739

Sure. Some of these cannot be processed by LV.

Would be good to switch back to something like "!getType()->isVoidTy()", once [VP]Types are introduced into VPlan/VPValue.

1479

Very well.

[fhahn]

Thanks. I'll prepare the follow-ups shortly.

llvm/lib/Transforms/Vectorize/VPlan.h

739

Sounds good!

llvm/lib/Transforms/Vectorize/VPlan.h

public:
 
   /// Return true if this instruction may modify memory.
   bool mayWriteToMemory() const {
     // TODO: we can use attributes of the called function to rule out memory
     //       modifications.
     return Opcode == Instruction::Store || Opcode == Instruction::Call ||
            Opcode == Instruction::Invoke || Opcode == SLPStore;
   }
+
+  bool hasResult() const {
+    if (const Value *UV = getUnderlyingValue())
+      return !UV->getType()->isVoidTy();
+    // Conservatively assume all VPInstructions without underlying values
+    // produce results.
+    return true;
+  }
 };
 
 /// VPWidenRecipe is a recipe for producing a copy of vector type for each
 /// Instruction in its ingredients independently, in order. This recipe covers
 /// most of the traditional vectorization cases where each ingredient transforms
 /// into a vectorized version of itself.
 class VPWidenRecipe : public VPRecipeBase {
 private:
public:
   /// Add \p CBV to the vector of condition bit values.
   void addCBV(VPValue *CBV) {
     VPCBVs.push_back(CBV);
   }
 
   void addVPValue(Value *V) {
     assert(V && "Trying to add a null Value to VPlan");
     assert(!Value2VPValue.count(V) && "Value already exists in VPlan");
     Value2VPValue[V] = new VPValue();
   }
 
   VPValue *getVPValue(Value *V) {
     assert(V && "Trying to get the VPValue of a null Value");
     assert(Value2VPValue.count(V) && "Value does not exist in VPlan");
     return Value2VPValue[V];
   }
 

llvm/lib/Transforms/Vectorize/VPlan.cpp

 
 void VPInstruction::print(raw_ostream &O) const {
   VPSlotTracker SlotTracker(getParent()->getPlan());
   print(O, SlotTracker);
 }
 
 void VPInstruction::print(raw_ostream &O, VPSlotTracker &SlotTracker) const {
   printAsOperand(O, SlotTracker);
+  if (hasResult())
     O << " = ";
 
   switch (getOpcode()) {
   case VPInstruction::Not:
     O << "not";
     break;
   case VPInstruction::ICmpULE:
     O << "icmp ule";
     break;
if (!Plan.getName().empty())
     OS << "\\n" << DOT::EscapeString(Plan.getName());
   if (!Plan.Value2VPValue.empty() || Plan.BackedgeTakenCount) {
     OS << ", where:";
     if (Plan.BackedgeTakenCount) {
       OS << "\\n";
       Plan.BackedgeTakenCount->print(OS, SlotTracker);
       OS << " := BackedgeTakenCount";
     }
     for (auto Entry : Plan.Value2VPValue) {
       OS << "\\n";
       Entry.second->print(OS, SlotTracker);
       OS << DOT::EscapeString(" := ");
       Entry.first->printAsOperand(OS, false);
     }
   }
   OS << "\"]\n";
   OS << "node [shape=rect, fontname=Courier, fontsize=30]\n";

 void VPValue::replaceAllUsesWith(VPValue *New) {
   for (VPUser *User : users())
     for (unsigned I = 0, E = User->getNumOperands(); I < E; ++I)
       if (User->getOperand(I) == this)
         User->setOperand(I, New);
 }
 
 void VPValue::printAsOperand(raw_ostream &OS, VPSlotTracker &Tracker) const {
+  if (const Value *UV = getUnderlyingValue()) {
+    if (UV->getType()->isVoidTy())
+      return;
+    // Get a string of underlying value from printAsOperand, so we can also handle numbered IR values without a name.
+    std::string UnderlyingName;
+    {
+      raw_string_ostream StrStream(UnderlyingName);
+      UV->printAsOperand(StrStream, false);
+      StrStream.flush();
+    }
+    if (isa<Constant>(UV))
+      OS << "ir constant " << UnderlyingName;
+    else
+      // Drop the % from the name.
+      OS << "%vp.ir." << StringRef(UnderlyingName).drop_front(1);
+    return;
+  }
+
   unsigned Slot = Tracker.getSlot(this);
   if (Slot == unsigned(-1))
     OS << "<badref>";
   else
-    OS << "%vp" << Tracker.getSlot(this);
+    OS << "%vp." << Tracker.getSlot(this);
 }
 
 void VPInterleavedAccessInfo::visitRegion(VPRegionBlock *Region,
                                           Old2NewTy &Old2New,
                                           InterleavedAccessInfo &IAI) {
   ReversePostOrderTraversal<VPBlockBase *> RPOT(Region->getEntry());
   for (VPBlockBase *Base : RPOT) {
     visitBlock(Base, Old2New, IAI);

 VPInterleavedAccessInfo::VPInterleavedAccessInfo(VPlan &Plan,
                                                  InterleavedAccessInfo &IAI) {
   Old2NewTy Old2New;
   visitRegion(cast<VPRegionBlock>(Plan.getEntry()), Old2New, IAI);
 }
 
 void VPSlotTracker::assignSlot(const VPValue *V) {
   assert(Slots.find(V) == Slots.end() && "VPValue already has a slot!");
+  const Value *UV = V->getUnderlyingValue();
+  if (!UV || !UV->hasName())
     Slots[V] = NextSlot++;
 }
 
 void VPSlotTracker::assignSlots(const VPBlockBase *VPBB) {
   if (auto *Region = dyn_cast<VPRegionBlock>(VPBB))
     assignSlots(Region);
   else
     assignSlots(cast<VPBasicBlock>(VPBB));
 }

llvm/lib/Transforms/Vectorize/VPlanValue.h

 // flow into, within and out of the VPlan. VPValues can stand for live-ins
 // coming from the input IR, instructions which VPlan will generate if executed
 // and live-outs which the VPlan will need to fix accordingly.
 class VPValue {
   friend class VPBuilder;
   friend class VPlanTransforms;
   friend class VPBasicBlock;
   friend class VPInterleavedAccessInfo;
+  friend class VPSlotTracker;
 
 private:
   const unsigned char SubclassID; ///< Subclass identifier (for isa/dyn_cast).
 
   SmallVector<VPUser *, 1> Users;
 
 protected:
   // Hold the underlying Value, if any, attached to this VPValue.
   Value *UnderlyingVal;
 
   VPValue(const unsigned char SC, Value *UV = nullptr)
       : SubclassID(SC), UnderlyingVal(UV) {}
 
   // DESIGN PRINCIPLE: Access to the underlying IR must be strictly limited to
   // the front-end and back-end of VPlan so that the middle-end is as
   // independent as possible of the underlying IR. We grant access to the
   // underlying IR using friendship. In that way, we should be able to use VPlan
   // for multiple underlying IRs (Polly?) by providing a new VPlan front-end,
   // back-end and analysis information for the new IR.
 
   /// Return the underlying Value attached to this VPValue.
   Value *getUnderlyingValue() { return UnderlyingVal; }
+  const Value *getUnderlyingValue() const { return UnderlyingVal; }
 
   // Set \p Val as the underlying Value of this VPValue.
   void setUnderlyingValue(Value *Val) {
     assert(!UnderlyingVal && "Underlying Value is already set.");
     UnderlyingVal = Val;
   }
 
 public:

llvm/unittests/Transforms/Vectorize/VPlanHCFGTest.cpp

 //===- llvm/unittest/Transforms/Vectorize/VPlanHCFGTest.cpp ---------------===//
 //
 // Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
 // See https://llvm.org/LICENSE.txt for license information.
 // SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
 //
 //===----------------------------------------------------------------------===//
 
 #include "../lib/Transforms/Vectorize/VPlan.h"
 #include "../lib/Transforms/Vectorize/VPlanTransforms.h"
 #include "VPlanTestBase.h"
 #include "gtest/gtest.h"
+#include <string>
 
 namespace llvm {
 namespace {
 
 class VPlanHCFGTest : public VPlanTestBase {};
 
 TEST_F(VPlanHCFGTest, testBuildHCFGInnerLoop) {
   const char *ModuleString =
TEST_F(VPlanHCFGTest, testBuildHCFGInnerLoop) {
   EXPECT_EQ(Phi, IndvarAdd->getOperand(0));
 
   VPInstruction *ICmp = dyn_cast<VPInstruction>(&*Iter++);
   EXPECT_EQ(Instruction::ICmp, ICmp->getOpcode());
   EXPECT_EQ(2u, ICmp->getNumOperands());
   EXPECT_EQ(IndvarAdd, ICmp->getOperand(0));
   EXPECT_EQ(VecBB->getCondBit(), ICmp);
 
+  std::string FullDump;
+  raw_string_ostream(FullDump) << *Plan;
+  EXPECT_EQ(R"(digraph VPlan {
+graph [labelloc=t, fontsize=30; label="Vectorization Plan"]
+node [shape=rect, fontname=Courier, fontsize=30]
+edge [fontname=Courier, fontsize=30]
+compound=true
+  subgraph cluster_N0 {
+    fontname=Courier
+    label="\<x1\> TopRegion"
+    N1 [label =
+      "entry:\n"
+    ]
+    N1 -> N2 [ label=""]
+    N2 [label =
+      "for.body:\n" +
+        "EMIT %vp.ir.indvars.iv = phi ir constant 0 %vp.ir.indvars.iv.next\l" +
+        "EMIT %vp.ir.arr.idx = getelementptr %vp.ir.A %vp.ir.indvars.iv\l" +
+        "EMIT %vp.ir.l1 = load %vp.ir.arr.idx\l" +
+        "EMIT %vp.ir.res = add %vp.ir.l1 ir constant 10\l" +
+        "EMIT store %vp.ir.res %vp.ir.arr.idx\l" +
+        "EMIT %vp.ir.indvars.iv.next = add %vp.ir.indvars.iv ir constant 1\l" +
+        "EMIT %vp.ir.exitcond = icmp %vp.ir.indvars.iv.next %vp.ir.N\l" +
+         "CondBit: %vp.ir.exitcond (for.body)\l"
+    ]
+    N2 -> N2 [ label="T"]
+    N2 -> N3 [ label="F"]
+    N3 [label =
+      "for.end:\n" +
+        "EMIT ret\l"
+    ]
+  }
+}
+)",
+            FullDump);
+
   LoopVectorizationLegality::InductionList Inductions;
   SmallPtrSet<Instruction *, 1> DeadInstructions;
   VPlanTransforms::VPInstructionsToVPRecipes(LI->getLoopFor(LoopHeader), Plan,
                                              Inductions, DeadInstructions);
 }
 
 TEST_F(VPlanHCFGTest, testVPInstructionToVPRecipesInner) {
   const char *ModuleString =

llvm/unittests/Transforms/Vectorize/VPlanTest.cpp

TEST(VPBasicBlockTest, print) {
 
   EXPECT_EQ(R"(digraph VPlan {
 graph [labelloc=t, fontsize=30; label="Vectorization Plan"]
 node [shape=rect, fontname=Courier, fontsize=30]
 edge [fontname=Courier, fontsize=30]
 compound=true
   N0 [label =
     ":\n" +
-      "EMIT %vp0 = catchswitch\l" +
-      "EMIT %vp1 = ret %vp0\l" +
-      "EMIT %vp2 = br %vp0 %vp1\l"
+      "EMIT %vp.0 = catchswitch\l" +
+      "EMIT %vp.1 = ret %vp.0\l" +
+      "EMIT %vp.2 = br %vp.0 %vp.1\l"
   ]
   N0 -> N1 [ label=""]
   N1 [label =
     ":\n" +
-      "EMIT %vp3 = indirectbr %vp2 %vp1\l" +
-      "EMIT %vp4 = invoke %vp0\l"
+      "EMIT %vp.3 = indirectbr %vp.2 %vp.1\l" +
+      "EMIT %vp.4 = invoke %vp.0\l"
   ]
 }
 )",
             FullDump);
 
   {
     std::string I3Dump;
     raw_string_ostream OS(I3Dump);
     I3->print(OS);
     OS.flush();
-    EXPECT_EQ("%vp2 = br %vp0 %vp1", I3Dump);
+    EXPECT_EQ("%vp.2 = br %vp.0 %vp.1", I3Dump);
   }
 
   {
     std::string I2Dump;
     raw_string_ostream OS(I2Dump);
     OS << *I2;
     OS.flush();
-    EXPECT_EQ("%vp1 = ret %vp0", I2Dump);
+    EXPECT_EQ("%vp.1 = ret %vp.0", I2Dump);
   }
 }
 
 } // namespace
 } // namespace llvm