This is an archive of the discontinued LLVM Phabricator instance.

Preserve inbounds information of GEP during Argument Promotion Pass across callee and its callers.
Needs ReviewPublic

Authored by pravinjagtap on Feb 7 2022, 3:39 AM.

Details

Summary

Presently inbounds information is lost between callee and its callers when Argument is promoted.
This change respects the inbounds of GEP emitted by front-end for callee and emits GEP with preserved
inbounds information in callers instead taking conservative approach of emitting GEP without inbounds
by default.

Note that Argument is only promoted when its *only* loaded (i.e. no store) in Argument Promotion Pass.

Motivation for this change is that the AMDGPUPromoteKernelArgumentsPass (https://reviews.llvm.org/D111464)
can infer the address-spaces and promote FLAT memory operation to GLOBAL based on this change.

Diff Detail

Event Timeline

pravinjagtap created this revision.Feb 7 2022, 3:39 AM
pravinjagtap requested review of this revision.Feb 7 2022, 3:39 AM
Herald added a project: Restricted Project. · View Herald TranscriptFeb 7 2022, 3:39 AM
pravinjagtap edited the summary of this revision. (Show Details)Feb 7 2022, 3:43 AM
nikic added a reviewer: nikic.Feb 7 2022, 6:24 AM
nikic added a subscriber: nikic.

It looks like you need to update a number of existing tests.

You should also test the case where the GEP inbounds is conditional -- I think it might still be okay based on a dereferenceabiliy argument, but it's not entirely obvious to me.

llvm/lib/Transforms/IPO/ArgumentPromotion.cpp
283

I think using inbounds here is okay, but the justification is not correct. You probably want to argue about byval-implied dereferenceability here.

It looks like you need to update a number of existing tests.

You should also test the case where the GEP inbounds is conditional -- I think it might still be okay based on a dereferenceabiliy argument, but it's not entirely obvious to me.

Thank you for your inputs. I am not sure whether I fully understood what do you mean by GEP inbounds is conditional. I am assuming that test should include: GEP with inbound and without inbound in callee so that other conditional branch will be exercised.

pravinjagtap planned changes to this revision.Feb 8 2022, 5:39 AM
pravinjagtap marked an inline comment as done.

Addressed the comments by nikic and fixed the unit tests

ormris removed a subscriber: ormris.Feb 8 2022, 9:46 AM
nikic added a comment.Feb 8 2022, 11:43 AM

It looks like you need to update a number of existing tests.

You should also test the case where the GEP inbounds is conditional -- I think it might still be okay based on a dereferenceabiliy argument, but it's not entirely obvious to me.

Thank you for your inputs. I am not sure whether I fully understood what do you mean by GEP inbounds is conditional. I am assuming that test should include: GEP with inbound and without inbound in callee so that other conditional branch will be exercised.

Sorry for the confusion: What I meant is a case where the GEP + load is not guaranteed to execute on entry to the function, but is behind a conditional branch.

Applied git-clang-format on change. Sorry for inconvenience.

Added test which exercises the conditional GEP inbound

It looks like you need to update a number of existing tests.

You should also test the case where the GEP inbounds is conditional -- I think it might still be okay based on a dereferenceabiliy argument, but it's not entirely obvious to me.

Thank you for your inputs. I am not sure whether I fully understood what do you mean by GEP inbounds is conditional. I am assuming that test should include: GEP with inbound and without inbound in callee so that other conditional branch will be exercised.

Added test /llvm/test/Transforms/ArgumentPromotion/conditional-gep-and-load.ll which exercises the conditional inbounds @nikic.

Using IsArgGepInBound map just to hold a boolean seems expensive to me, can you just add a field to OriginalLoads value?

llvm/lib/Transforms/IPO/ArgumentPromotion.cpp
201

UI isn't expected to be null here, dyn_cast should be enough.

Addressed vpykhtin's commnets

I would replace pair of pair with ordinary struct, much easier to read the code

Herald added a project: Restricted Project. · View Herald TranscriptOct 20 2022, 12:18 AM
Herald added a subscriber: kosarev. · View Herald Transcript