Page MenuHomePhabricator

[AAPointerInfo] handle multiple offsets in PHI
ClosedPublic

Authored by sameerds on Nov 30 2022, 2:07 AM.

Details

Summary

The arguments to a PHI may represent a recurrence by eventually using the output
of the PHI itself. This is now handled by checking for cycles in the control
flow. If a PHI is not in a recurrence, it is now able to report multiple offsets
instead of conservatively reporting unknown.

Diff Detail

Event Timeline

sameerds created this revision.Nov 30 2022, 2:07 AM
Herald added a project: Restricted Project. · View Herald TranscriptNov 30 2022, 2:07 AM
sameerds requested review of this revision.Nov 30 2022, 2:07 AM
Herald added a reviewer: sstefan1. · View Herald Transcript
Herald added a project: Restricted Project. · View Herald Transcript
foad added inline comments.Nov 30 2022, 2:33 AM
llvm/lib/Transforms/IPO/AttributorAttributes.cpp
1524

Typo "is can"

llvm/test/CodeGen/AMDGPU/implicitarg-attributes.ll
31

Have they?!

jdoerfert added inline comments.Nov 30 2022, 10:24 AM
llvm/include/llvm/Transforms/IPO/Attributor.h
1131

Do we really need legacy PM support? I would prefer not to add complexity for it.

arsenm added inline comments.Nov 30 2022, 10:29 AM
llvm/include/llvm/Transforms/IPO/Attributor.h
1131

Yes. Contrary to popular belief, the new PM is not complete. Until we have codegen support, we need legacy PM support for IR passes run in the backend

aeubanks added inline comments.
llvm/include/llvm/Transforms/IPO/Attributor.h
1131

is it possible to run whatever pass you're interested in as part of the optimization pipeline?

arsenm added inline comments.Nov 30 2022, 10:53 AM
llvm/include/llvm/Transforms/IPO/Attributor.h
1131

Not in the right place. We can't even move all the IR passes because of the one RegionPass which new PM doesn't have.

jdoerfert added inline comments.Nov 30 2022, 11:15 AM
llvm/include/llvm/Transforms/IPO/Attributor.h
1131

Shouldn't we invest the time and code to provide a "region pass wrapper"? We stopped testing most of these things with the old PM, using it is not a great idea.

arsenm added inline comments.Nov 30 2022, 11:21 AM
llvm/include/llvm/Transforms/IPO/Attributor.h
1131

We should invest the time in killing of the structurizer entirely. It being a region pass is a problem for many reasons. But that's hard

jdoerfert added inline comments.Dec 1 2022, 7:41 AM
llvm/include/llvm/Transforms/IPO/Attributor.h
1131

@arsenm Where do I find that structurizer and what should the replacement do/look like? It's not https://github.com/llvm/llvm-project/blob/main/llvm/lib/Transforms/Scalar/StructurizeCFG.cpp right?

arsenm added a comment.Dec 1 2022, 9:45 AM

Can someone look at D83612, D85168, D92781, D83607 and D83613? We seem to have the base parts for new PM codegen, but they've been languishing in review waiting. We're stuck waiting for these with no direction

llvm/include/llvm/Transforms/IPO/Attributor.h
1131

That is the one.

arsenm added inline comments.
llvm/include/llvm/Transforms/IPO/Attributor.h
1131

Trying to write a replacement structurizer is basically a rite of passage; many have tried and most have failed.

It needs to be a function pass. @ruiling was looking into writing a new one recently. @bogner may also have another implementation already

arsenm added a subscriber: asbirlea.Dec 1 2022, 9:53 AM

Can someone look at D83612, D85168, D92781, D83607 and D83613? We seem to have the base parts for new PM codegen, but they've been languishing in review waiting. We're stuck waiting for these with no direction

cc @aeubanks @asbirlea

jdoerfert accepted this revision.Dec 7 2022, 8:28 AM

I'm not thrilled about the old PM stuff and we really need to remove it asap, but it's good to go in once the dependence lands.

llvm/lib/Transforms/IPO/AttributorAttributes.cpp
1524

^

llvm/test/CodeGen/AMDGPU/implicitarg-attributes.ll
31

^

This revision is now accepted and ready to land.Dec 7 2022, 8:28 AM
arsenm added inline comments.Dec 7 2022, 9:18 AM
llvm/test/CodeGen/AMDGPU/implicitarg-attributes.ll
66

I rather dislike CHECK-NOT checks for their fragility, and it's best to be as vague as possible. Probably should remove the .value_kind

This revision was landed with ongoing or failed builds.Dec 14 2022, 7:19 PM
This revision was automatically updated to reflect the committed changes.
sameerds marked 5 inline comments as done.