[PATCH] D17965: [NVPTX] Adds a new address space inference pass.
Justin Lebar via llvm-commits
llvm-commits at lists.llvm.org
Tue Mar 15 18:43:03 PDT 2016
jlebar added a comment.
Almost all of my comments are cosmetic, but I have a few nontrivial questions. This looks quite sane to me.
================
Comment at: lib/Target/NVPTX/NVPTXInferAddressSpaces.cpp:257
@@ +256,3 @@
+// ValueWithNewAddrSpace. In that case, uses undef as a placeholder operand and
+// adds that operand use to UndefUsesToFix so that caller can fix them later.
+Value *cloneInstructionWithNewAddressSpace(
----------------
We don't necessarily clone `I` -- e.g. if it's an AddrSpaceCast where the pointer types match. Probably worth documenting, since it's significant that this function returns a Value and not an Instruction.
================
Comment at: lib/Target/NVPTX/NVPTXInferAddressSpaces.cpp:268
@@ +267,3 @@
+ // Because `I` is generic, the source address space must be specific.
+ // Therefore, the inferred address space must be the source space according
+ // to our algorithm.
----------------
"space, according"
================
Comment at: lib/Target/NVPTX/NVPTXInferAddressSpaces.cpp:272
@@ +271,3 @@
+ if (Src->getType() != NewPtrType)
+ return new BitCastInst(Src, NewPtrType);
+ return Src;
----------------
Maybe it's obvious, but I can't figure out when we hit this. Src->getType()'s address space matches NewAddrSpace, but Src->getType() != NewPtrType?
================
Comment at: lib/Target/NVPTX/NVPTXInferAddressSpaces.cpp:278
@@ +277,3 @@
+ SmallVector<Value*, 4> NewPointerOperands;
+ for (unsigned OperandNo = 0; OperandNo < I->getNumOperands(); ++OperandNo) {
+ if (I->getOperand(OperandNo)->getType()->isPointerTy()) {
----------------
Any reason we can't iterate over I->operands() directly? getOperandUse() seems to do exactly the same thing as getOperand, just with different types.
http://llvm.org/docs/doxygen/html/User_8h_source.html#l00143
If we do have to do a non-range-based for loop, style guide says to cache the length (which I find silly, but there it is).
================
Comment at: lib/Target/NVPTX/NVPTXInferAddressSpaces.cpp:283
@@ +282,3 @@
+ &I->getOperandUse(OperandNo), NewAddrSpace, ValueWithNewAddrSpace,
+ UndefUsesToFix);
+ }
----------------
Would push_back be clearer?
================
Comment at: lib/Target/NVPTX/NVPTXInferAddressSpaces.cpp:291
@@ +290,3 @@
+ case Instruction::PHI: {
+ PHINode *PHI = cast<PHINode>(I);
+ PHINode *NewPHI = PHINode::Create(NewPtrType, PHI->getNumIncomingValues());
----------------
Is it worth asserting that the phi's type is a pointer? Otherwise we're in trouble here.
================
Comment at: lib/Target/NVPTX/NVPTXInferAddressSpaces.cpp:302
@@ +301,3 @@
+ GetElementPtrInst *GEP = cast<GetElementPtrInst>(I);
+ SmallVector<Value*, 4> NewIndexList;
+ for (auto Iter = GEP->idx_begin(); Iter != GEP->idx_end(); ++Iter)
----------------
NewIndexList(GEP->idx_begin(), GEP->idx_end());
?
================
Comment at: lib/Target/NVPTX/NVPTXInferAddressSpaces.cpp:313
@@ +312,3 @@
+ }
+}
+
----------------
I may be missing something, but it seems like this would be a lot simpler if we instead cloned the old instruction, then iterated over its relevant operands and converted the pointer ones to the new address space. Then we wouldn't have to worry about tricky things like remembering to call setInBounds.
In fact if we wanted, I think we could write this mostly-generically:
* for each instruction type, make a list of all of the operand indices that may contain pointers and that we want to modify.
* then iterate over that list and convert the ones which are pointers to the new address space.
================
Comment at: lib/Target/NVPTX/NVPTXInferAddressSpaces.cpp:326
@@ +325,3 @@
+ // Because CE is generic, the source address space must be specific.
+ // Therefore, the inferred address space must be the source space according
+ // to our algorithm.
----------------
"space, according"
================
Comment at: lib/Target/NVPTX/NVPTXInferAddressSpaces.cpp:341
@@ +340,3 @@
+ // bitcast, and getelementptr) do not incur cycles in the data flow graph
+ // and that (2) this function is called on constant expressions in
+ // postorder.
----------------
s/and that/and /
(List parallelism -- you should be able to read the list while omitting (1): "because foo and (2) this function is called ...", not "and that".)
================
Comment at: lib/Target/NVPTX/NVPTXInferAddressSpaces.cpp:352
@@ +351,3 @@
+ if (CE->getOpcode() == Instruction::GetElementPtr) {
+ // Needs to specify the source type when construct a getelementptr constant
+ // expression.
----------------
when constructing
================
Comment at: lib/Target/NVPTX/NVPTXInferAddressSpaces.cpp:355
@@ +354,3 @@
+ return CE->getWithOperands(
+ NewOperands, TargetType, false,
+ NewOperands[0]->getType()->getPointerElementType());
----------------
perhaps document what 'false' is? Unless it's obvious. (Old rant: http://jlebar.com/2011/12/16/Boolean_parameters_to_API_functions_considered_harmful..html)
================
Comment at: lib/Target/NVPTX/NVPTXInferAddressSpaces.cpp:362
@@ +361,3 @@
+
+// Returns a clone of the value `V` with its operands replaced as specified in
+// ValueWithNewAddrSpace. This function is called on every generic address
----------------
value `V`, with its
================
Comment at: lib/Target/NVPTX/NVPTXInferAddressSpaces.cpp:364
@@ +363,3 @@
+// ValueWithNewAddrSpace. This function is called on every generic address
+// expressions whose address space needs to be modified in postorder.
+//
----------------
modified, in postorder
(Otherwise it looks like "in postorder" modifies "needs to be modified", rather than "is called".)
================
Comment at: lib/Target/NVPTX/NVPTXInferAddressSpaces.cpp:371
@@ +370,3 @@
+ SmallVectorImpl<Use *> *UndefUsesToFix) {
+ // Every values in Postorder are generic address expressions.
+ assert(isAddressExpression(*V) &&
----------------
"Every value in Postorder is", or "All values in Postorder are"
================
Comment at: lib/Target/NVPTX/NVPTXInferAddressSpaces.cpp:419
@@ +418,3 @@
+
+bool NVPTXInferAddressSpaces::runOnFunction(Function &F) {
+ // Collects all generic address expressions in postorder.
----------------
I haven't written any large passes, so if this is wrong, feel free to ignore, but carrying the InferredAddressSpaces map as a member variable seems like it might add more complexity than using it as a local variable here. In particular, we wouldn't need to remember to clear it, and updateAddressSpace could simply return the new address space, instead of updating the global map. (Maybe it would return an Optional<unsigned>, or it could just return the same address space to indicate "no change".)
================
Comment at: lib/Target/NVPTX/NVPTXInferAddressSpaces.cpp:461
@@ +460,3 @@
+ // Our algorithm only updates the address spaces of generic address
+ // expressions which are those in InferredAddrSpace.
+ if (!InferredAddrSpace.count(User))
----------------
expressions, which
================
Comment at: lib/Target/NVPTX/NVPTXInferAddressSpaces.cpp:469
@@ +468,3 @@
+ if (InferredAddrSpace.lookup(User) ==
+ AddressSpace::ADDRESS_SPACE_GENERIC)
+ continue;
----------------
Is it worth not doing the hash lookup twice?
================
Comment at: lib/Target/NVPTX/NVPTXInferAddressSpaces.cpp:535
@@ +534,3 @@
+ for (Value *V : Postorder) {
+ if (Value *NewV = ValueWithNewAddrSpace.lookup(V)) {
+ SmallVector<Use *, 4> Uses;
----------------
Although style guide likes declaring variables inside if statements, it also likes reducing nesting, and in this case maybe that wins -- i.e., reverse the if statement and add a continue?
================
Comment at: lib/Target/NVPTX/NVPTXInferAddressSpaces.cpp:538
@@ +537,3 @@
+ for (Use &U : V->uses())
+ Uses.push_back(&U);
+ DEBUG(dbgs() << "Replacing the uses of " << *V << "\n to\n " << *NewV
----------------
Why can't we iterate over V->uses() directly? (Is it because we're modifying them?)
================
Comment at: lib/Target/NVPTX/NVPTXInferAddressSpaces.cpp:544
@@ +543,3 @@
+ (isa<StoreInst>(U->getUser()) && U->getOperandNo() == 1)) {
+ // If V is used as the pointer operand of load/store, sets the pointer
+ // operand to NewV. This replacement does not change the element type
----------------
of a load/store
================
Comment at: lib/Target/NVPTX/NVPTXInferAddressSpaces.cpp:545
@@ +544,3 @@
+ // If V is used as the pointer operand of load/store, sets the pointer
+ // operand to NewV. This replacement does not change the element type
+ // so the resultant load/store is valid.
----------------
type, so (joining independent clauses with a conjunction)
================
Comment at: lib/Target/NVPTX/NVPTXInferAddressSpaces.cpp:546
@@ +545,3 @@
+ // operand to NewV. This replacement does not change the element type
+ // so the resultant load/store is valid.
+ U->set(NewV);
----------------
"is still valid", maybe?
================
Comment at: lib/Target/NVPTX/NVPTXInferAddressSpaces.cpp:560
@@ +559,3 @@
+ BasicBlock::iterator InsertPos = std::next(I->getIterator());
+ while (isa<PHINode>(InsertPos))
+ ++InsertPos;
----------------
Maybe it's obvious, but it's not clear to me why we want to go past phi nodes. We're adding a cast to the value V, which is defined at position I->getIterator(). So surely that value is in fact available right after that position -- why skip further down?
================
Comment at: test/CodeGen/NVPTX/access-non-generic.ll:6
@@ -4,1 +5,3 @@
+; RUN: opt < %s -S -nvptx-infer-addrspace | FileCheck %s --check-prefix IR
+; RUN: opt < %s -S -nvptx-infer-addrspace | FileCheck %s --check-prefix IR-WITH-LOOP
----------------
Are these incompatible -- i.e., we can't combine these two into one FileCheck call with two use-prefixes?
http://reviews.llvm.org/D17965
More information about the llvm-commits
mailing list