This is an archive of the discontinued LLVM Phabricator instance.

[Attributor]Introduce call base context argument pathway.
Needs ReviewPublic

Authored by kuter on Jul 13 2020, 9:03 PM.

Details

Summary

This patch puts a constraint on the attributes that will receive call base context.

Diff Detail

Event Timeline

kuter created this revision.Jul 13 2020, 9:03 PM
Herald added a reviewer: sstefan1. · View Herald Transcript
Herald added a reviewer: uenoku. · View Herald Transcript
Herald added a reviewer: homerdin. · View Herald Transcript
Herald added a reviewer: baziotis. · View Herald Transcript
Herald added a project: Restricted Project. · View Herald Transcript
kuter edited the summary of this revision. (Show Details)Jul 13 2020, 9:05 PM
jdoerfert added inline comments.Jul 18 2020, 6:12 PM
llvm/include/llvm/Transforms/IPO/Attributor.h
1416

Nit: For better or worse we use F and Fn as a variable name for functions usually.

I read the comment and do not know what happens. We need to improve the wording, maybe elaborate.

llvm/lib/Transforms/IPO/Attributor.cpp
96

I'm confused by the description. Is the analysis invaliding values?

807

Nit: Assertions always with messages.


Can we have a explanation here or in the header what is happening here. The commit message should also contain more details.

815

Style: No braces. Use a range loop or part of the constructor (or use append).

kuter updated this revision to Diff 279068.Jul 19 2020, 12:39 AM

fix wording. use pointer_iterator instead of for loop.
fix naming.

kuter marked 3 inline comments as done.Jul 19 2020, 12:41 AM
kuter added inline comments.
llvm/lib/Transforms/IPO/Attributor.cpp
815

That was my initial idea but since arg_begin() is a pointer the SmallVector was trying to converyt Argument to Value*. After investigating I found that there is something called pointer_iterator that would fix this issue.

kuter marked 3 inline comments as done.Jul 19 2020, 12:25 PM
kuter updated this revision to Diff 281190.Jul 28 2020, 4:56 AM

add unit test.
fix bug (😄)
fix typo.

kuter retitled this revision from [Attributor][WIP]Introduce call base context argument pathway. to [Attributor]Introduce call base context argument pathway..Jul 28 2020, 4:57 AM
kuter edited the summary of this revision. (Show Details)
kuter updated this revision to Diff 281191.Jul 28 2020, 5:00 AM
kuter edited the summary of this revision. (Show Details)

remove unused header.

kuter added inline comments.Jul 29 2020, 9:09 PM
llvm/unittests/Transforms/IPO/AttributorTestBase.h
53 ↗(On Diff #281191)

contents BumpPtrAllocator will be freed after this block is dead.
This causes a use after free.

jdoerfert added inline comments.Jul 29 2020, 9:16 PM
llvm/unittests/Transforms/IPO/AttributorTestBase.h
53 ↗(On Diff #281191)

Put the allocator in the same scope as the Attributor?

kuter added inline comments.Jul 29 2020, 9:19 PM
llvm/unittests/Transforms/IPO/AttributorTestBase.h
53 ↗(On Diff #281191)

That's what I am planing to do. I just wanted to add a inline commend so I don't forget about this issue.