[PATCH] D48323: Derive GEP index type from Data Layout (cont)

[Matt Arsenault via Phabricator via llvm-commits]

arsenm added inline comments.


================
Comment at: lib/CodeGen/AsmPrinter/AsmPrinter.cpp:2071
     // Generate a symbolic expression for the byte address
-    APInt OffsetAI(getDataLayout().getPointerTypeSizeInBits(CE->getType()), 0);
+    APInt OffsetAI(getDataLayout().getIndexTypeSizeInBits(CE->getType()), 0);
     cast<GEPOperator>(CE)->accumulateConstantOffset(getDataLayout(), OffsetAI);
----------------
This shouldn't be using the default address space


================
Comment at: lib/Transforms/InstCombine/InstCombineCasts.cpp:1735
 
     Value *P = Builder.CreateZExtOrTrunc(CI.getOperand(0), Ty);
     return new IntToPtrInst(P, CI.getType());
----------------
delena wrote:
> lebedev.ri wrote:
> > delena wrote:
> > > lebedev.ri wrote:
> > > > I realize that it is existing behaviour, but this creates new extra instruction,
> > > > and does not check for uses of previous instruction.
> > > > This is not good for instcombine, although i suppose this is a very special case.
> > > > 
> > > > You //might// want to check that there are tests that verify this behavior, or add such tests.
> > > It shouldn't create an extra instruction. I fixed incompatibility between  IntPtrType and expected type of the source.
> > > The current version just entered to an endless loop, because the  IntPtrType  is the index type and not the pointer type.
> > Hm.
> > `@test1` had just `ptrtoint`, and ended up with `ptrtoint`+`and`+`icmp`
> > `@test2` had just `inttoptr`, and ended up with `trunc`+`inttoptr`
> > That to me looks like new instructions.
> > But like i said, i suspect this is a very special edge-case.
> and+icmp is not new. It is just a way to return i1.
> test2 returns another type. And it is common, nothing special here.
> 
The InstCombine change is a separate patch from the codegen changes


Repository:
  rL LLVM

https://reviews.llvm.org/D48323