Page MenuHomePhabricator

[ConstantFolding] Don't try to cast vectors to pointer if they have different size
AbandonedPublic

Authored by davide on Oct 27 2016, 7:35 PM.

Details

Summary

An attempt to fix the bug reported by Michael on D26014

Diff Detail

Event Timeline

davide updated this revision to Diff 76158.Oct 27 2016, 7:35 PM
davide retitled this revision from to [ConstantFolding] Don't try to cast vectors to pointer if they have different size.
davide updated this object.
davide added reviewers: majnemer, RKSimon, filcab, Bigcheese.
davide added a subscriber: llvm-commits.
filcab edited edge metadata.Oct 28 2016, 2:04 PM

Using undef in tests can hide some things.
Can you tell me what you get with your test case for this?

%struct.foo = type { [1 x i64] }

define <4 x i64*> @patatino3(%struct.foo*) {
  %2 = getelementptr inbounds %struct.foo, %struct.foo* %0, i32 0,
                               i32 0, <4 x i8> zeroinitializer
  ret <4 x i64*> %2
}

clang 3.9 gives me this:

define <4 x i64*> @patatino3(%struct.foo* readnone) local_unnamed_addr #0 {
  %2 = getelementptr inbounds %struct.foo, %struct.foo* %0, i64 0, i32 0, <4 x i64> zeroinitializer
  ret <4 x i64*> %2
}

Where the first i32 and the i8 vector have been cast to i64 and i64 vector.

lib/Analysis/ConstantFolding.cpp
731

Did you mean Ops[i]->getType()->isVectorTy()?

From DataLayout.h:

/// \brief Returns an integer type with size at least as big as that of a
/// pointer in the given address space.
IntegerType *getIntPtrType(LLVMContext &C, unsigned AddressSpace = 0) const;

So IntPtrTy will never be a vector type.
It seems with your initial patch, we're not casting at all. But I might be misunderstanding something here.

davide added inline comments.Oct 28 2016, 3:36 PM
lib/Analysis/ConstantFolding.cpp
731

There's a different overload which can actually return a VectorTy.

/// \brief Returns an integer (vector of integer) type with size at least as
/// big as that of a pointer of the given pointer (vector of pointer) type.
Type *getIntPtrType(Type *) const;

I also double checked and in my example IntPtrTy is a VectorTy

Using undef in tests can hide some things.
Can you tell me what you get with your test case for this?

%struct.foo = type { [1 x i64] }

define <4 x i64*> @patatino3(%struct.foo*) {
  %2 = getelementptr inbounds %struct.foo, %struct.foo* %0, i32 0,
                               i32 0, <4 x i8> zeroinitializer
  ret <4 x i64*> %2
}

clang 3.9 gives me this:

define <4 x i64*> @patatino3(%struct.foo* readnone) local_unnamed_addr #0 {
  %2 = getelementptr inbounds %struct.foo, %struct.foo* %0, i64 0, i32 0, <4 x i64> zeroinitializer
  ret <4 x i64*> %2
}

Where the first i32 and the i8 vector have been cast to i64 and i64 vector.

This is what I get

[davide@localhost bin]$ ./opt -instcombine filcab.ll -o - -S
; ModuleID = 'filcab.ll'
source_filename = "filcab.ll"

%struct.foo = type { [1 x i64] }

define <4 x i64*> @patatino3(%struct.foo*) {
  %2 = getelementptr inbounds %struct.foo, %struct.foo* %0, i64 0, i32 0, <4 x i64> zeroinitializer
  ret <4 x i64*> %2
}
[davide@localhost bin]$ cat filcab.ll
%struct.foo = type { [1 x i64] }

define <4 x i64*> @patatino3(%struct.foo*) {
  %2 = getelementptr inbounds %struct.foo, %struct.foo* %0, i32 0,
                               i32 0, <4 x i8> zeroinitializer
  ret <4 x i64*> %2
}

which I think is what is expected, but another pair of eyes wouldn't hurt

davide updated this revision to Diff 76267.Oct 28 2016, 3:58 PM
davide edited edge metadata.

On a second thought, the check for the size should be enough in this case.

RKSimon edited edge metadata.Dec 5 2016, 10:12 AM

What is happening with this?

davide abandoned this revision.Dec 7 2016, 12:15 PM

A similar fix is landing (https://reviews.llvm.org/D27389).