[PATCH] [opaque pointer type] Add textual IR support for explicit type parameter to getelementptr instruction
David Blaikie
dblaikie at gmail.com
Thu Feb 26 16:15:43 PST 2015
On Tue, Feb 24, 2015 at 7:43 PM, Duncan P. N. Exon Smith <
dexonsmith at apple.com> wrote:
>
> > On 2015-Feb-24, at 15:59, David Blaikie <dblaikie at gmail.com> wrote:
> >
> > ping
> >
> > On Fri, Feb 13, 2015 at 5:32 PM, David Blaikie <dblaikie at gmail.com>
> wrote:
> > Hi rafael, resistor, grosbach, chandlerc, grosser,
> >
> > One of several parallel first steps to remove the target type of
> pointers,
> > replacing them with a single opaque pointer type.
> >
> > This adds an explicit type parameter to the gep instruction so that when
> the
> > first parameter becomes an opaque pointer type, the type to gep through
> is
> > still available to the instructions.
> >
> > * This doesn't modify gep operators, only instructions (operators will be
> > handled separately)
> >
> > * Textual IR changes only. Bitcode (including upgrade) and changing the
> > in-memory representation will be in separate changes.
> >
> > * geps of vectors are transformed as:
> > getelementptr <4 x float*> %x, ...
> > ->getelementptr float, <4 x float*> %x, ...
> > Then, once the opaque pointer type is introduced, this will ultimately
> look
> > like:
> > getelementptr float, <4 x ptr> %x
> > with the unambiguous interpretation that it is a vector of pointers to
> float.
> >
> > * address spaces remain on the pointer, not the type:
> > getelementptr float addrspace(1)* %x
> > ->getelementptr float, float addrspace(1)* %x
> > Then, eventually:
> > getelementptr float, ptr addrspace(1) %x
> >
> > 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.
> >
> > update.py:
> > import fileinput
> > import sys
> > import re
> >
> > ib = re.compile("getelementptr inbounds +[^( ]")
> > norm = re.compile("getelementptr +[^( ]")
> >
> > ibrep = re.compile(r"(^.*?getelementptr inbounds )(((?:<\d* x )?)(.*?)(|
> addrspace\(\d\)) *\*(|>)(?:$|
> *(?:%|@|null|undef|blockaddress|getelementptr|addrspacecast|bitcast|inttoptr|\[\[[a-zA-Z]|\{\{).*$))")
> > normrep = re.compile( r"(^.*?getelementptr )(((?:<\d* x )?)(.*?)(|
> addrspace\(\d\)) *\*(|>)(?:$|
> *(?:%|@|null|undef|blockaddress|getelementptr|addrspacecast|bitcast|inttoptr|\[\[[a-zA-Z]|\{\{).*$))")
> >
> > def conv(match, line):
> > if not match:
> > return line
> > line = match.groups()[0]
> > if len(match.groups()[5]) == 0:
> > line += match.groups()[2]
> > line += match.groups()[3]
> > line += ", "
> > line += match.groups()[1]
> > line += "\n"
> > return line
> >
> > for line in sys.stdin:
> > if line.find("getelementptr ") == line.find("getelementptr inbounds"):
> > if line.find("getelementptr inbounds") != line.find("getelementptr
> inbounds ("):
> > line = conv(re.match(ibrep, line), line)
> > elif line.find("getelementptr ") != line.find("getelementptr ("):
> > line = conv(re.match(normrep, line), line)
> > sys.stdout.write(line)
> >
> > apply.sh:
> > #!/bin/bash
> > for name in "$@"
> > do
> > python3 `dirname "$0"`/update.py < "$name" > "$name.tmp" && mv
> "$name.tmp" "$name"
> > rm -f "$name.tmp"
> > done
> >
> > The actual commands:
> > From llvm/src:
> > find test/ -name *.ll | xargs ./apply.sh
> > From llvm/src/tools/clang:
> > find test/ -name *.mm -o -name *.m -o -name *.cpp -o -name *.c | xargs
> -I '{}' ../../apply.sh "{}"
> >
> > After that, check-all (with llvm, clang, clang-tools-extra, lld,
> compiler-rt
> > all checked out - I haven't looked at polly yet). I haven't included the
> actual
> > test changes in this review to make it easier to look at - but they are
> /only/
> > the changes created by this script (except the unit test change, which I
> have
> > included) - nothing manual.
> >
> > The extra 'rm' in the apply.sh script is due to a few files in clang's
> test
> > suite using interesting unicode stuff that my python script was throwing
> > exceptions on. None of those files needed to be migrated, so it seemed
> > sufficient to ignore those cases.
> >
> > http://reviews.llvm.org/D7636
> >
> > Files:
> > include/llvm/IR/Instructions.h
> > lib/AsmParser/LLParser.cpp
> > lib/IR/AsmWriter.cpp
> > unittests/IR/ConstantsTest.cpp
> >
> > Index: include/llvm/IR/Instructions.h
> > ===================================================================
> > --- include/llvm/IR/Instructions.h
> > +++ include/llvm/IR/Instructions.h
> > @@ -845,6 +845,13 @@
> > return cast<SequentialType>(Instruction::getType());
> > }
> >
> > + Type *getSourceElementType() const {
> > + SequentialType *Ty = cast<SequentialType>(getPointerOperandType());
> > + if (VectorType *VTy = dyn_cast<VectorType>(Ty))
> > + Ty = cast<SequentialType>(VTy->getElementType());
> > + return Ty->getElementType();
> > + }
> > +
> > /// \brief Returns the address space of this instruction's pointer
> type.
> > unsigned getAddressSpace() const {
> > // Note that this is always the same as the pointer operand's
> address space
> > Index: lib/AsmParser/LLParser.cpp
> > ===================================================================
> > --- lib/AsmParser/LLParser.cpp
> > +++ lib/AsmParser/LLParser.cpp
> > @@ -5364,8 +5364,18 @@
> >
> > bool InBounds = EatIfPresent(lltok::kw_inbounds);
> >
> > + Type *Ty = nullptr;
> > + if (ParseType(Ty)) return true;
> > + ParseToken(lltok::comma, "expected comma after getelementptr's type");
> > +
>
> You neglected to check the return of `ParseToken()` here. Like that
> patch, I think you should have a testcase for this, since this is how
> we're rejecting the old syntax.
>
*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.
>
> > if (ParseTypeAndValue(Ptr, Loc, PFS)) return true;
> >
> > +
> > + Type *PtrTy = Ptr->getType();
> > + if (VectorType *VT = dyn_cast<VectorType>(PtrTy))
> > + PtrTy = VT->getElementType();
> > + assert(Ty == cast<SequentialType>(PtrTy)->getElementType());
> > +
>
> This should be an `Error()`, and I think there should be a testcase
> for the error (a type mismatch seems easy enough to hit, so the
> diagnostic seems important to me).
>
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).
Fixed & Tested.
- David
>
> > Type *BaseType = Ptr->getType();
> > PointerType *BasePointerType =
> dyn_cast<PointerType>(BaseType->getScalarType());
> > if (!BasePointerType)
> > Index: lib/IR/AsmWriter.cpp
> > ===================================================================
> > --- lib/IR/AsmWriter.cpp
> > +++ lib/IR/AsmWriter.cpp
> > @@ -2880,6 +2880,11 @@
> > Out << ", ";
> > TypePrinter.print(I.getType(), Out);
> > } else if (Operand) { // Print the normal way.
> > + if (const GetElementPtrInst *GEP = dyn_cast<GetElementPtrInst>(&I))
> {
> > + Out << ' ';
> > + TypePrinter.print(GEP->getSourceElementType(), Out);
> > + Out << ',';
> > + }
> >
> > // PrintAllTypes - Instructions who have operands of all the same
> type
> > // omit the type from all but the first operand. If the
> instruction has
> > Index: unittests/IR/ConstantsTest.cpp
> > ===================================================================
> > --- unittests/IR/ConstantsTest.cpp
> > +++ unittests/IR/ConstantsTest.cpp
> > @@ -246,9 +246,9 @@
> > // FIXME: getGetElementPtr() actually creates an inbounds ConstantGEP,
> > // not a normal one!
> > //CHECK(ConstantExpr::getGetElementPtr(Global, V, false),
> > - // "getelementptr i32** @dummy, i32 1");
> > + // "getelementptr i32*, i32** @dummy, i32 1");
> > CHECK(ConstantExpr::getInBoundsGetElementPtr(Global, V),
> > - "getelementptr inbounds i32** @dummy, i32 1");
> > + "getelementptr inbounds i32*, i32** @dummy, i32 1");
> >
> > CHECK(ConstantExpr::getExtractElement(P6, One), "extractelement <2 x
> i16> "
> > P6STR ", i32 1");
> >
> > EMAIL PREFERENCES
> > http://reviews.llvm.org/settings/panel/emailpreferences/
> >
> > _______________________________________________
> > llvm-commits mailing list
> > llvm-commits at cs.uiuc.edu
> > http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits
> >
> >
> > _______________________________________________
> > llvm-commits mailing list
> > llvm-commits at cs.uiuc.edu
> > http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits
>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20150226/3eb7bc17/attachment.html>
More information about the llvm-commits
mailing list