<div dir="ltr"><br><div class="gmail_extra"><br><div class="gmail_quote">On Tue, Feb 24, 2015 at 7:43 PM, Duncan P. N. Exon Smith <span dir="ltr"><<a href="mailto:dexonsmith@apple.com" target="_blank">dexonsmith@apple.com</a>></span> wrote:<br><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div class="HOEnZb"><div class="h5"><br>
> On 2015-Feb-24, at 15:59, David Blaikie <<a href="mailto:dblaikie@gmail.com">dblaikie@gmail.com</a>> wrote:<br>
><br>
> ping<br>
><br>
> On Fri, Feb 13, 2015 at 5:32 PM, David Blaikie <<a href="mailto:dblaikie@gmail.com">dblaikie@gmail.com</a>> wrote:<br>
> Hi rafael, resistor, grosbach, chandlerc, grosser,<br>
><br>
> One of several parallel first steps to remove the target type of pointers,<br>
> replacing them with a single opaque pointer type.<br>
><br>
> This adds an explicit type parameter to the gep instruction so that when the<br>
> first parameter becomes an opaque pointer type, the type to gep through is<br>
> still available to the instructions.<br>
><br>
> * This doesn't modify gep operators, only instructions (operators will be<br>
> handled separately)<br>
><br>
> * Textual IR changes only. Bitcode (including upgrade) and changing the<br>
> in-memory representation will be in separate changes.<br>
><br>
> * geps of vectors are transformed as:<br>
> getelementptr <4 x float*> %x, ...<br>
> ->getelementptr float, <4 x float*> %x, ...<br>
> Then, once the opaque pointer type is introduced, this will ultimately look<br>
> like:<br>
> getelementptr float, <4 x ptr> %x<br>
> with the unambiguous interpretation that it is a vector of pointers to float.<br>
><br>
> * address spaces remain on the pointer, not the type:<br>
> getelementptr float addrspace(1)* %x<br>
> ->getelementptr float, float addrspace(1)* %x<br>
> Then, eventually:<br>
> getelementptr float, ptr addrspace(1) %x<br>
><br>
> Importantly, the massive amount of test case churn has been automated by same crappy python code. I had to manually update a few test cases that wouldn't fit the script's model (r228970,r229196,r229197,r229198). The python script just massages stdin and writes the result to stdout, I then wrapped that in a shell script to handle replacing files, then using the usual find+xargs to migrate all the files.<br>
><br>
> update.py:<br>
> import fileinput<br>
> import sys<br>
> import re<br>
><br>
> ib = re.compile("getelementptr inbounds +[^( ]")<br>
> norm = re.compile("getelementptr +[^( ]")<br>
><br>
> ibrep = re.compile(r"(^.*?getelementptr inbounds )(((?:<\d* x )?)(.*?)(| addrspace\(\d\)) *\*(|>)(?:$| *(?:%|@|null|undef|blockaddress|getelementptr|addrspacecast|bitcast|inttoptr|\[\[[a-zA-Z]|\{\{).*$))")<br>
> normrep = re.compile( r"(^.*?getelementptr )(((?:<\d* x )?)(.*?)(| addrspace\(\d\)) *\*(|>)(?:$| *(?:%|@|null|undef|blockaddress|getelementptr|addrspacecast|bitcast|inttoptr|\[\[[a-zA-Z]|\{\{).*$))")<br>
><br>
> def conv(match, line):<br>
> if not match:<br>
> return line<br>
> line = match.groups()[0]<br>
> if len(match.groups()[5]) == 0:<br>
> line += match.groups()[2]<br>
> line += match.groups()[3]<br>
> line += ", "<br>
> line += match.groups()[1]<br>
> line += "\n"<br>
> return line<br>
><br>
> for line in sys.stdin:<br>
> if line.find("getelementptr ") == line.find("getelementptr inbounds"):<br>
> if line.find("getelementptr inbounds") != line.find("getelementptr inbounds ("):<br>
> line = conv(re.match(ibrep, line), line)<br>
> elif line.find("getelementptr ") != line.find("getelementptr ("):<br>
> line = conv(re.match(normrep, line), line)<br>
> sys.stdout.write(line)<br>
><br>
> apply.sh:<br>
> #!/bin/bash<br>
> for name in "$@"<br>
> do<br>
> python3 `dirname "$0"`/update.py < "$name" > "$name.tmp" && mv "$name.tmp" "$name"<br>
> rm -f "$name.tmp"<br>
> done<br>
><br>
> The actual commands:<br>
> From llvm/src:<br>
> find test/ -name *.ll | xargs ./apply.sh<br>
> From llvm/src/tools/clang:<br>
> find test/ -name *.mm -o -name *.m -o -name *.cpp -o -name *.c | xargs -I '{}' ../../apply.sh "{}"<br>
><br>
> After that, check-all (with llvm, clang, clang-tools-extra, lld, compiler-rt<br>
> all checked out - I haven't looked at polly yet). I haven't included the actual<br>
> test changes in this review to make it easier to look at - but they are /only/<br>
> the changes created by this script (except the unit test change, which I have<br>
> included) - nothing manual.<br>
><br>
> The extra 'rm' in the apply.sh script is due to a few files in clang's test<br>
> suite using interesting unicode stuff that my python script was throwing<br>
> exceptions on. None of those files needed to be migrated, so it seemed<br>
> sufficient to ignore those cases.<br>
><br>
> <a href="http://reviews.llvm.org/D7636" target="_blank">http://reviews.llvm.org/D7636</a><br>
><br>
> Files:<br>
> include/llvm/IR/Instructions.h<br>
> lib/AsmParser/LLParser.cpp<br>
> lib/IR/AsmWriter.cpp<br>
> unittests/IR/ConstantsTest.cpp<br>
><br>
> Index: include/llvm/IR/Instructions.h<br>
> ===================================================================<br>
> --- include/llvm/IR/Instructions.h<br>
> +++ include/llvm/IR/Instructions.h<br>
> @@ -845,6 +845,13 @@<br>
> return cast<SequentialType>(Instruction::getType());<br>
> }<br>
><br>
> + Type *getSourceElementType() const {<br>
> + SequentialType *Ty = cast<SequentialType>(getPointerOperandType());<br>
> + if (VectorType *VTy = dyn_cast<VectorType>(Ty))<br>
> + Ty = cast<SequentialType>(VTy->getElementType());<br>
> + return Ty->getElementType();<br>
> + }<br>
> +<br>
> /// \brief Returns the address space of this instruction's pointer type.<br>
> unsigned getAddressSpace() const {<br>
> // Note that this is always the same as the pointer operand's address space<br>
> Index: lib/AsmParser/LLParser.cpp<br>
> ===================================================================<br>
> --- lib/AsmParser/LLParser.cpp<br>
> +++ lib/AsmParser/LLParser.cpp<br>
> @@ -5364,8 +5364,18 @@<br>
><br>
> bool InBounds = EatIfPresent(lltok::kw_inbounds);<br>
><br>
> + Type *Ty = nullptr;<br>
> + if (ParseType(Ty)) return true;<br>
> + ParseToken(lltok::comma, "expected comma after getelementptr's type");<br>
> +<br>
<br>
</div></div>You neglected to check the return of `ParseToken()` here. Like that<br>
patch, I think you should have a testcase for this, since this is how<br>
we're rejecting the old syntax.<br></blockquote><div><br>*nod* (curious that the error message is completely ignored since I didn't return from here - I guess the error message gets overwritten by the next ParseTypeAndValue call... ) - fixed.<br> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
<span class=""><br>
> if (ParseTypeAndValue(Ptr, Loc, PFS)) return true;<br>
><br>
> +<br>
> + Type *PtrTy = Ptr->getType();<br>
> + if (VectorType *VT = dyn_cast<VectorType>(PtrTy))<br>
> + PtrTy = VT->getElementType();<br>
> + assert(Ty == cast<SequentialType>(PtrTy)->getElementType());<br>
> +<br>
<br>
</span>This should be an `Error()`, and I think there should be a testcase<br>
for the error (a type mismatch seems easy enough to hit, so the<br>
diagnostic seems important to me).<br></blockquote><div><br>Ah, good point (I was on the fence about this since this error case won't be around for too long & shouldn't make it into a release - but yeah, for all teh people migrating tests, etc, certainly makes sense).<br><br>Fixed & Tested.<br><br>- David<br> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
<div class="HOEnZb"><div class="h5"><br>
> Type *BaseType = Ptr->getType();<br>
> PointerType *BasePointerType = dyn_cast<PointerType>(BaseType->getScalarType());<br>
> if (!BasePointerType)<br>
> Index: lib/IR/AsmWriter.cpp<br>
> ===================================================================<br>
> --- lib/IR/AsmWriter.cpp<br>
> +++ lib/IR/AsmWriter.cpp<br>
> @@ -2880,6 +2880,11 @@<br>
> Out << ", ";<br>
> TypePrinter.print(I.getType(), Out);<br>
> } else if (Operand) { // Print the normal way.<br>
> + if (const GetElementPtrInst *GEP = dyn_cast<GetElementPtrInst>(&I)) {<br>
> + Out << ' ';<br>
> + TypePrinter.print(GEP->getSourceElementType(), Out);<br>
> + Out << ',';<br>
> + }<br>
><br>
> // PrintAllTypes - Instructions who have operands of all the same type<br>
> // omit the type from all but the first operand. If the instruction has<br>
> Index: unittests/IR/ConstantsTest.cpp<br>
> ===================================================================<br>
> --- unittests/IR/ConstantsTest.cpp<br>
> +++ unittests/IR/ConstantsTest.cpp<br>
> @@ -246,9 +246,9 @@<br>
> // FIXME: getGetElementPtr() actually creates an inbounds ConstantGEP,<br>
> // not a normal one!<br>
> //CHECK(ConstantExpr::getGetElementPtr(Global, V, false),<br>
> - // "getelementptr i32** @dummy, i32 1");<br>
> + // "getelementptr i32*, i32** @dummy, i32 1");<br>
> CHECK(ConstantExpr::getInBoundsGetElementPtr(Global, V),<br>
> - "getelementptr inbounds i32** @dummy, i32 1");<br>
> + "getelementptr inbounds i32*, i32** @dummy, i32 1");<br>
><br>
> CHECK(ConstantExpr::getExtractElement(P6, One), "extractelement <2 x i16> "<br>
> P6STR ", i32 1");<br>
><br>
> EMAIL PREFERENCES<br>
> <a href="http://reviews.llvm.org/settings/panel/emailpreferences/" target="_blank">http://reviews.llvm.org/settings/panel/emailpreferences/</a><br>
><br>
> _______________________________________________<br>
> llvm-commits mailing list<br>
> <a href="mailto:llvm-commits@cs.uiuc.edu">llvm-commits@cs.uiuc.edu</a><br>
> <a href="http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits" target="_blank">http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits</a><br>
><br>
><br>
> _______________________________________________<br>
> llvm-commits mailing list<br>
> <a href="mailto:llvm-commits@cs.uiuc.edu">llvm-commits@cs.uiuc.edu</a><br>
> <a href="http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits" target="_blank">http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits</a><br>
<br>
</div></div></blockquote></div><br></div></div>