[llvm] r247570 - [MergeFuncs] Fix bug in merging GetElementPointers

[JF Bastien via llvm-commits]

On Mon, Sep 14, 2015 at 11:25 AM, David Blaikie <dblaikie at gmail.com> wrote:

>
>
> On Mon, Sep 14, 2015 at 11:13 AM, JF Bastien <jfb at google.com> wrote:
>
>> GetElementPointers must have the first argument's type compared
>>>> for structural equivalence. Previously the code erroneously compared the
>>>> pointer's type, but this code was dead because all pointer types (of the
>>>> same address space) are the same. The pointee must be compared instead
>>>> (using the type stored in the GEP, not from the pointer type which will
>>>> be erased anyway).
>>>>
>>>
>>> Thanks so much for being aware of this/using APIs that'll be forwards
>>> compatible with the eventual removal of typed pointers.
>>>
>>> Just in case: do you care about the GEP pointer operands address space
>>> (I assume not) or vector types (eg: a gep over a vector of pointers)? I
>>> assume not, but just checking - both those aspects of the pointer operand
>>> are still in the pointer operand's type, not in the source element type.
>>>
>>
>> We do care about address spaces in general: we can't merge two functions
>> if they refer to different address spaces. I'm not sure that's what you're
>> asking for though: in the end we (roughly) only want to merge functions
>> that would codegen to the same code. I think the same applies to your
>> vector of pointers question?
>>
>
> My point was - do you care if these two GEPs refer to different pointers
> from address spaces? What if the GEPs are differently vectored (if vector
> GEPs can exist in this codepath, etc)?
>

Yes, we care if the address spaces are different. That's checked at the
start of the function.

By vectored you mean the indexing? I think this is handled properly, but it
would be great if you could confirm.


If you do care about the (in)equality of address space or vector-ness in
> these two GEPs, you'll need to compare those - those would've previously
> been caught by just comparing the two GEPs pointer operand types, but won't
> be any more with this change.
>


For reference, the current code is:

int FunctionComparator::cmpGEPs(const GEPOperator *GEPL,
                               const GEPOperator *GEPR) {

  unsigned int ASL = GEPL->getPointerAddressSpace();
  unsigned int ASR = GEPR->getPointerAddressSpace();

  if (int Res = cmpNumbers(ASL, ASR))
    return Res;

  // When we have target data, we can reduce the GEP down to the value in
bytes
  // added to the address.
  const DataLayout &DL = FnL->getParent()->getDataLayout();
  unsigned BitWidth = DL.getPointerSizeInBits(ASL);
  APInt OffsetL(BitWidth, 0), OffsetR(BitWidth, 0);
  if (GEPL->accumulateConstantOffset(DL, OffsetL) &&
      GEPR->accumulateConstantOffset(DL, OffsetR))
    return cmpAPInts(OffsetL, OffsetR);
  if (int Res = cmpTypes(GEPL->getSourceElementType(),
                         GEPR->getSourceElementType()))
    return Res;

  if (int Res = cmpNumbers(GEPL->getNumOperands(), GEPR->getNumOperands()))
    return Res;

  for (unsigned i = 0, e = GEPL->getNumOperands(); i != e; ++i) {
    if (int Res = cmpValues(GEPL->getOperand(i), GEPR->getOperand(i)))
      return Res;
  }

  return 0;
}
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20150914/5d3056a0/attachment.html>