This is an archive of the discontinued LLVM Phabricator instance.

[AggressiveInstCombine] Fix cases where non-opaque pointers are used
ClosedPublic

Authored by dstuttard on Oct 5 2022, 2:05 AM.

Details

Summary

In the case of non-opaque pointers, when combining consecutive loads,
need to bitcast the pointer source to the combined type size, otherwise
asserts are triggered.

Diff Detail

Event Timeline

dstuttard created this revision.Oct 5 2022, 2:05 AM
Herald added a project: Restricted Project. · View Herald TranscriptOct 5 2022, 2:05 AM
Herald added a subscriber: hiraditya. · View Herald Transcript
dstuttard requested review of this revision.Oct 5 2022, 2:05 AM
Herald added a project: Restricted Project. · View Herald TranscriptOct 5 2022, 2:05 AM
nikic added a reviewer: nikic.Oct 5 2022, 2:25 AM
nikic added a subscriber: nikic.

Please remove the typed pointer test case. I'm willing to accept the patch at this time, but not the test case.

llvm/lib/Transforms/AggressiveInstCombine/AggressiveInstCombine.cpp
814

Checking for opaque pointers is not necessary, the bitcast will just not be emitted in that case.

817

You can sue getPointerAddressSpace() here and save the cast.

824

Extract the IntegerType into a variable for reuse. It is used a third time in the isTypeLegal check above.

bipmis added inline comments.Oct 5 2022, 3:14 AM
llvm/lib/Transforms/AggressiveInstCombine/AggressiveInstCombine.cpp
816

We already have the Pointer Address space defined for the Load Instruction. So the below should suffice

Type *NewTy = IntegerType::get(Load1Ptr->getContext(), LOps.LoadSize);
Value *NewPtr = Builder.CreateBitCast(Load1Ptr, NewTy->getPointerTo());
NewLoad = Builder.CreateAlignedLoad(NewTy, NewPtr, LI1->getAlign(),
                                    LI1->isVolatile(), "");
dstuttard marked 3 inline comments as done.Oct 5 2022, 4:04 AM

Thanks - removed the test and made the changes.

dstuttard updated this revision to Diff 465337.Oct 5 2022, 4:10 AM

Updated for review requests

nikic added inline comments.Oct 5 2022, 5:01 AM
llvm/lib/Transforms/AggressiveInstCombine/AggressiveInstCombine.cpp
816

I don't think that's entirely correct, use still need to pass AS to getPointerTo().

dstuttard updated this revision to Diff 465351.Oct 5 2022, 5:32 AM

Include address space AS in the pointerto call

dstuttard marked 2 inline comments as done.Oct 5 2022, 5:32 AM
nikic accepted this revision.Oct 5 2022, 5:35 AM

LGTM

This revision is now accepted and ready to land.Oct 5 2022, 5:35 AM
This revision was landed with ongoing or failed builds.Oct 5 2022, 5:44 AM
This revision was automatically updated to reflect the committed changes.