Page MenuHomePhabricator

[IVDescriptors] Make pointer inductions compatible with opaque pointers
ClosedPublic

Authored by nikic on Jun 23 2021, 10:12 AM.

Details

Summary

Store the used element type in the InductionDescriptor. For typed pointers, it remains the pointer element type. For opaque pointers, we always use an i8 element type, such that the step is a simple offset.

A previous version of this patch instead tried to guess the element type from the induction GEP, but this is not reliable, as the GEP may be hidden (see @both in iv_outside_user.ll).

Diff Detail

Event Timeline

nikic created this revision.Jun 23 2021, 10:12 AM
nikic requested review of this revision.Jun 23 2021, 10:12 AM
Herald added a project: Restricted Project. · View Herald TranscriptJun 23 2021, 10:12 AM

I'm not familiar with this code, but looks fine overall with one nit

llvm/lib/Analysis/IVDescriptors.cpp
985

should this be (IK == IK_PtrInduction) == (ElementType != nullptr)?

nikic updated this revision to Diff 354514.Jun 25 2021, 8:29 AM
nikic edited the summary of this revision. (Show Details)

Adjust assertion.

nikic added inline comments.Jun 25 2021, 8:30 AM
llvm/lib/Analysis/IVDescriptors.cpp
985

I feel like that's a bit hard to understand. What do you think about the new version that spells out the two possibilities explicitly?

The main alternative I see here is to use different logic for opaque pointers and always base them off an i8 element type

Can we do that based on the fact the we can't deduce the element type from GEP step instruction (e.g. if there is no obvious one)?

llvm/lib/Analysis/IVDescriptors.cpp
1269

Would it work if we'd have a chain of geps with different srcElementTypes? E.g. something like

%ptr = phi [ %from.preheader ], [ ptr.next ]
%gep = getelementptr i16, %ptr, 1
%ptr.next = getelementptr i32, %gep, 1

The step isn't even representable in multiples of i32 in that scenario.

aeubanks accepted this revision.Jul 15 2021, 10:31 AM

not too familiar with this code, but seems ok

llvm/lib/Analysis/IVDescriptors.cpp
1276

this is already checked by the verifier

This revision is now accepted and ready to land.Jul 15 2021, 10:31 AM
a.elovikov added inline comments.Jul 15 2021, 10:37 AM
llvm/lib/Analysis/IVDescriptors.cpp
1269

@aeubanks , why can't this happen? Or, if it can, why isn't it an issue?

aeubanks added inline comments.Jul 15 2021, 10:40 AM
llvm/lib/Analysis/IVDescriptors.cpp
1269

I'm not super familiar with this code, but it looks like each InductionDescriptor describes a single induction. So each GEP would have its own InductionDescriptor?

nikic updated this revision to Diff 369539.Aug 30 2021, 1:37 PM
nikic edited the summary of this revision. (Show Details)

Always use i8 element type for opaque pointers. I've come around to the idea that with opaque pointers, we should always use i8 GEPs for constant offsets. While in this particular case we can actually make a fairly good guess at a reasonable type to use, this is not possible in other cases, and for canonicalization reasons it's best to use the same approach everywhere.

can we make everything use i8 now? that'd be one less thing to worry about during the migration step

can we make everything use i8 now? that'd be one less thing to worry about during the migration step

Without opaque pointers, that would require inserting a bitcast before and after every GEP (unless it already happens to work on an i8 pointer). It's possible, but I don't think it makes sense with typed pointers.

I don't see any i8 geps in the test, is that expected?

llvm/include/llvm/Analysis/IVDescriptors.h
360

add TODO that this will no longer be necessary after opaque pointers

nikic added a comment.Sep 1 2021, 12:49 AM

I don't see any i8 geps in the test, is that expected?

The test currently just checks that we don't crash. To perform vectorization the GEPs need to be inbounds, which requires opaque pointer support in LAA first.

nikic updated this revision to Diff 369944.Sep 1 2021, 8:48 AM
nikic marked an inline comment as done.

Add TODO

This revision was landed with ongoing or failed builds.Sep 1 2021, 12:02 PM
This revision was automatically updated to reflect the committed changes.