Page MenuHomePhabricator

[IVDescriptors] Make pointer inductions compatible with opaque pointers
AcceptedPublic

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

Details

Reviewers
fhahn
spatel
aeubanks
Group Reviewers
Restricted Project
Summary

Rather than inspecting the element type of the pointer, look for the GEP step instruction and take it's element type. Also store that in the IVDescriptor and fetch it in LoopVectorize.

A problem of this approach is that it fails if there is no direct GEP step instruction. It's usually present, but one LoopVectorize test shows a case where it isn't. I'm not sure how important it is.

The main alternative I see here is to use different logic for opaque pointers and always base them off an i8 element type (aka "uglygep"). The disadvantage of that is that a switch to opaque pointers would have a non-trivial impact on how optimization works here, so I think it would be the worse option. Another variant is to always use i8 element types, i.e. make this purely offset based and force necessary pointer casts for non-opaque pointers.

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
1272

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.Thu, Jul 15, 10:31 AM

not too familiar with this code, but seems ok

llvm/lib/Analysis/IVDescriptors.cpp
1279

this is already checked by the verifier

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

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

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

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?