[PATCH] D49024: [Polly] [WIP] Introduce ShapeInfo into polly for sizes and strides.
Philip Pfaffe via Phabricator via llvm-commits
llvm-commits at lists.llvm.org
Thu Aug 2 06:48:30 PDT 2018
philip.pfaffe added a comment.
On top of my generic concern regarding the unclean implementation of the ShapeInfo itself, I left you a bunch of inline comments, mostly concerning style.
================
Comment at: include/polly/CodeGen/IslExprBuilder.h:97
+ /// A map from const SCEV* to llvm::Values
+ typedef llvm::MapVector<const llvm::SCEV *, llvm::AssertingVH<llvm::Value>>
+ SCEVToValueTy;
----------------
Why is this a MapVector instead of a DenseMap?
================
Comment at: include/polly/ScopInfo.h:291
+
+raw_ostream &operator<<(raw_ostream &OS, const ShapeInfo &Shape);
+
----------------
Make this an inline friend?
================
Comment at: include/polly/ScopInfo.h:673
/// The sizes of each dimension as SCEV*.
- SmallVector<const SCEV *, 4> DimensionSizes;
+ // SmallVector<const SCEV *, 4> DimensionSizes;
----------------
This and the comment should be removed.
================
Comment at: include/polly/Support/ScopHelper.h:486
+/// The name of the array index "intrinsic" interpreted by polly
+static const std::string POLLY_ABSTRACT_INDEX_BASENAME = "polly_array_index";
+
----------------
StringRef?
================
Comment at: lib/Analysis/ScopBuilder.cpp:686
+
+ if ((Call->getNumArgOperands()) % 2 != 1) {
+ return false;
----------------
What's the point of this check?
================
Comment at: lib/Analysis/ScopBuilder.cpp:700
+
+ const SCEV *OffsetSCEV = [&] {
+ Value *Offset = Call->getArgOperand(0);
----------------
Pointless IILE;
================
Comment at: lib/Analysis/ScopBuilder.cpp:749
+ Type *ElementType = Val->getType();
+ assert(BasePtr);
+ assert(ElementType);
----------------
What's the point of these asserts?
================
Comment at: lib/Analysis/ScopBuilder.cpp:1608
+ BP, BP->getType(), false, {AF},
+ ShapeInfo::fromSizes({nullptr}), GlobalRead);
}
----------------
Why {nullptr}?
================
Comment at: lib/Analysis/ScopDetection.cpp:546
+ if (Call && Call->getCalledFunction() &&
+ Call->getCalledFunction()->getName().count(
+ POLLY_ABSTRACT_INDEX_BASENAME))
----------------
Why `count()`?
================
Comment at: lib/Analysis/ScopDetection.cpp:574
+
+ if (isSCEVMultidimArrayAccess(S)) {
+ return true;
----------------
Superfluous braces.
================
Comment at: lib/Analysis/ScopInfo.cpp:1075
: Kind(MemoryKind::Array), AccType(AccType), Statement(Stmt),
- InvalidDomain(nullptr), AccessRelation(nullptr),
- NewAccessRelation(AccRel), FAD(nullptr) {
+ InvalidDomain(nullptr), Shape(ShapeInfo::fromSizes({nullptr})),
+ AccessRelation(nullptr), NewAccessRelation(AccRel), FAD(nullptr) {
----------------
Is the `nullptr` intentional?
================
Comment at: lib/Analysis/ScopInfo.cpp:4023
+Value *getPointerFromLoadOrStore(Value *V) {
+ if (LoadInst *LI = dyn_cast<LoadInst>(V))
----------------
Should be static.
================
Comment at: lib/Analysis/ScopInfo.cpp:4041
+ // Yes this is nuts. Yes I stil want to do this.
+ auto unifyStridedArrayBasePtrs = [&]() -> Value * {
+ if (Shape.hasSizes())
----------------
There is no need to make this an IILE.
================
Comment at: lib/Analysis/ScopInfo.cpp:4065
+
+ // errs() << "Creating: " << (int)Kind << "\n";
+ // BasePtr->dump();
----------------
Unrelated.
================
Comment at: lib/Analysis/ScopInfo.cpp:4097
+ errs() << "---\n";
+ // report_fatal_error("SAI was given sizes when it had strides");
+ return SAI.get();
----------------
This should really be an error.
================
Comment at: lib/CodeGen/IslExprBuilder.cpp:292
- IndexOp = nullptr;
- for (unsigned u = 1, e = isl_ast_expr_get_op_n_arg(Expr); u < e; u++) {
- Value *NextIndex = create(isl_ast_expr_get_op_arg(Expr, u));
- assert(NextIndex->getType()->isIntegerTy() &&
- "Access index should be an integer");
+ IndexOp1 = nullptr;
+ IndexOp2 = nullptr;
----------------
This entire thing should be broken up into //at least// three functions.
================
Comment at: lib/CodeGen/IslNodeBuilder.cpp:1216
+ for (unsigned i = 0; i < Array->getNumberOfDimensions(); i++) {
+ isl_pw_aff *ParametricPwAff = Array->getDimensionSizePw(i).release();
+ assert(ParametricPwAff && "parametric pw_aff corresponding "
----------------
Prefer using the c++ API.
================
Comment at: lib/CodeGen/PPCGCodeGeneration.cpp:147
+
+ Function *F = [&]() {
+ Function *Existing = nullptr;
----------------
I really don't like IILE patterns. It doesn't even save you structure here.
================
Comment at: lib/CodeGen/PPCGCodeGeneration.cpp:884
Value *DevArray = createCallAllocateMemoryForDevice(ArraySize);
+ // Value *DevArray =
+ // createCallAllocateMemoryForDevice(Builder.getInt64(200));
----------------
Unrelated.
================
Comment at: lib/CodeGen/PPCGCodeGeneration.cpp:1184
Value *ArraySize = ConstantInt::get(Builder.getInt64Ty(), Array->size);
+ // const auto SAI = (ScopArrayInfo *)(Array->user);
----------------
Unrelated.
================
Comment at: lib/CodeGen/PPCGCodeGeneration.cpp:1192
+ // const unsigned int StartIndex = SAI->hasStrides() ? 0 : 1;
for (unsigned int i = 1; i < Array->n_index; i++) {
----------------
Unrelated.
================
Comment at: lib/CodeGen/PPCGCodeGeneration.cpp:1290
createCallCopyFromHostToDevice(HostPtr, DevPtr, Size);
+ // createCallCopyFromHostToDevice(HostPtr, DevPtr, Builder.getInt64(200));
else
----------------
Unrelated.
================
Comment at: lib/CodeGen/PPCGCodeGeneration.cpp:1293
createCallCopyFromDeviceToHost(DevPtr, HostPtr, Size);
+ // createCallCopyFromDeviceToHost(DevPtr, HostPtr, Builder.getInt64(200));
----------------
Unrelated.
================
Comment at: lib/CodeGen/PPCGCodeGeneration.cpp:1549
+// return whether `Array` is used in `Kernel` or not.
+bool isArrayUsedInKernel(ScopArrayInfo *Array, ppcg_kernel *Kernel,
+ gpu_prog *Prog) {
----------------
Should be static.
================
Comment at: lib/CodeGen/PPCGCodeGeneration.cpp:1884
assert(Clone && "Expected cloned function to be initialized.");
- assert(ValueMap.find(Fn) == ValueMap.end() &&
- "Fn already present in ValueMap");
+ // assert(ValueMap.find(Fn) == ValueMap.end() &&
+ // "Fn already present in ValueMap");
----------------
This seems unsound.
================
Comment at: lib/CodeGen/PPCGCodeGeneration.cpp:1890
+ Module *Host = S.getFunction().getParent();
+ for (int i = 0; i <= 4; i++) {
+ const std::string BaseName = POLLY_ABSTRACT_INDEX_BASENAME;
----------------
Why 4? This needs documentation.
================
Comment at: lib/CodeGen/PPCGCodeGeneration.cpp:2148
+ // TODO: this is so fugly, find a better way to express this :(
+ ShapeInfo NewShape = [&](SmallVector<const SCEV *, 4> &Sizes) {
+ if (SAI->hasStrides()) {
----------------
What does this code even do?
================
Comment at: lib/CodeGen/PPCGCodeGeneration.cpp:3686
Value *RTC = NodeBuilder.createRTC(Condition);
- Builder.GetInsertBlock()->getTerminator()->setOperand(0, RTC);
+ Builder.GetInsertBlock()->getTerminator()->setOperand(0,
+ Builder.getTrue());
----------------
This seems like an unsound or at least unrelated change.
================
Comment at: lib/Support/ScopHelper.cpp:708
+ if (!MaybeBitcast)
+ return Optional<std::pair<CallInst *, GEPOperator *>>(None);
+
----------------
You can just say None.
================
Comment at: lib/Support/ScopHelper.cpp:723
+ if (!GEP)
+ return Optional<std::pair<CallInst *, GEPOperator *>>(None);
+
----------------
See above.
================
Comment at: lib/Support/ScopHelper.cpp:726
+ auto *MaybeCall = GEP->getOperand(GEP->getNumOperands() - 1);
+ assert(MaybeCall);
+
----------------
Why would the operand be null?
================
Comment at: lib/Support/ScopHelper.cpp:731
+ if (!Call)
+ return Optional<std::pair<CallInst *, GEPOperator *>>(None);
+
----------------
See above.
================
Comment at: lib/Support/ScopHelper.cpp:733
+
+ if (!Call->getCalledFunction()->getName().count(
+ POLLY_ABSTRACT_INDEX_BASENAME))
----------------
Why do you count() instead of equality compare?
================
Comment at: lib/Support/ScopHelper.cpp:738
+ std::pair<CallInst *, GEPOperator *> p = std::make_pair(Call, GEP);
+ return Optional<std::pair<CallInst *, GEPOperator *>>(p);
+}
----------------
Just return the make_pair.
https://reviews.llvm.org/D49024
More information about the llvm-commits
mailing list