[PATCH] D109584: Implementing expansion pass for VP load and store.

[Craig Topper via Phabricator via llvm-commits]

craig.topper added inline comments.


================
Comment at: llvm/lib/CodeGen/ExpandVectorPredication.cpp:404
+  assert(VPI.canIgnoreVectorLengthParam());
+  auto &I = cast<Instruction>(VPI);
+
----------------
VPIntrinsic is a subclass of Instruction, we shouldn't need an explicit cast.


================
Comment at: llvm/lib/CodeGen/ExpandVectorPredication.cpp:406
+
+  auto MaskParam = VPI.getMaskParam();
+  auto PtrParam = VPI.getMemoryPointerParam();
----------------
Don't use auto here.


================
Comment at: llvm/lib/CodeGen/ExpandVectorPredication.cpp:416
+  default:
+    abort(); // not a VP memory intrinsic
+
----------------
Why not llvm_unreachable?


================
Comment at: llvm/lib/CodeGen/ExpandVectorPredication.cpp:474
+  IRBuilder<> Builder(InsertPt);
+  Builder.SetCurrentDebugLocation(InsertPt->getDebugLoc());
+  Value *SrcEltPtr = Builder.CreatePointerCast(
----------------
Doesn't IRBuilder's constructor also set the debug location?


================
Comment at: llvm/lib/CodeGen/ExpandVectorPredication.cpp:477
+      Src, EltTy->getPointerTo(Src->getType()->getPointerAddressSpace()));
+  auto *SubvecSrc = Builder.CreateInBoundsGEP(EltTy, SrcEltPtr, Offset);
+  Value *VResult = Dest;
----------------
Don't use auto.


================
Comment at: llvm/lib/CodeGen/ExpandVectorPredication.cpp:481
+    Value *vi = ConstantInt::get(OffsetTy, i);
+    auto *EltOffset = Builder.CreateAdd(Offset, vi);
+    auto *EltPtr = Builder.CreateInBoundsGEP(EltTy, SubvecSrc, vi);
----------------
Don't use auto


================
Comment at: llvm/lib/CodeGen/ExpandVectorPredication.cpp:500
+  IRBuilder<> Builder(InsertPt);
+  Builder.SetCurrentDebugLocation(InsertPt->getDebugLoc());
+  Value *DestEltPtr = Builder.CreatePointerCast(
----------------
Does't IRBuilder's constructor set the debug location?


================
Comment at: llvm/lib/CodeGen/ExpandVectorPredication.cpp:544
+
+  auto &I = cast<Instruction>(VPI);
+
----------------
Shouldn't be needed


================
Comment at: llvm/lib/CodeGen/ExpandVectorPredication.cpp:553
+  Value *NewMemoryInst = nullptr;
+  char const *Prefix;
+
----------------
Use StringRef


================
Comment at: llvm/lib/CodeGen/ExpandVectorPredication.cpp:557
+  default:
+    abort(); // not a VP load or store
+
----------------
llvm_unreachable


================
Comment at: llvm/lib/CodeGen/ExpandVectorPredication.cpp:567
+
+  bool isLoad = (OC == Instruction::Load);
+
----------------
Capitalize


================
Comment at: llvm/lib/CodeGen/ExpandVectorPredication.cpp:584
+      NewMemoryInst = NewLoad;
+    } break;
+    case Instruction::Store: {
----------------
break should be inside the curly braces


================
Comment at: llvm/lib/CodeGen/ExpandVectorPredication.cpp:590
+      NewMemoryInst = NewStore;
+    } break;
+    default:
----------------
Same here


================
Comment at: llvm/lib/CodeGen/ExpandVectorPredication.cpp:603
+
+  Value *VResult = (isLoad ? UndefValue::get(VecTy) : nullptr);
+
----------------
Drop parentheses around the statement


================
Comment at: llvm/lib/CodeGen/ExpandVectorPredication.cpp:609
+  SplitBlockAndInsertIfThenElse(Pred, &I, &ShortTerm, &LongTerm);
+  ShortTerm->getParent()->setName(Twine(Prefix) + "short");
+  LongTerm->getParent()->setName(Twine(Prefix) + "long");
----------------
Using StringRef for Prefix should eliminate the need for Twine constructor here


================
Comment at: llvm/lib/CodeGen/ExpandVectorPredication.cpp:716
+      return true;
+    else
+      return false;
----------------
Drop else after return


================
Comment at: llvm/lib/CodeGen/ExpandVectorPredication.cpp:723
+      return true;
+    else
+      return false;
----------------
Drop else after return


================
Comment at: llvm/lib/CodeGen/ExpandVectorPredication.cpp:782
+  default:
+    abort(); // unexpected intrinsic
+  case Intrinsic::vp_load:
----------------
llvm_unreachable


================
Comment at: llvm/lib/CodeGen/ExpandVectorPredication.cpp:787
+      return expandPredicationInUnfoldedLoadStore(Builder, VPI);
+    } else {
+      return expandPredicationInMemoryIntrinsic(Builder, VPI);
----------------
Drop else after return and drop the curly braces for the if body


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D109584/new/

https://reviews.llvm.org/D109584