This is an archive of the discontinued LLVM Phabricator instance.

[Attributor] New attribute to identify what byte ranges are alive for an allocation
Needs ReviewPublic

Authored by vidsinghal on Aug 3 2023, 9:31 PM.

Details

Summary

Changes the size of allocations automatically.
Only implements the case when a single range from start of the allocation is alive.

Diff Detail

Event Timeline

vidsinghal created this revision.Aug 3 2023, 9:31 PM
Herald added a project: Restricted Project. · View Herald TranscriptAug 3 2023, 9:31 PM
Herald added subscribers: hoy, ormris, okura and 2 others. · View Herald Transcript

High level: Merge the test in one file. Use descriptive names for functions. remove the printf and other stuff that is not useful.

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

Manifest is only called if the state is valid at the end. It looks like you duplicate lots of checks here that should not be necessary.

12878–12883

Assertion, this should never be the case, right?

12885–12887

The lambda is called only once, right? Why use a lambda at all?
Also, if the alloca cannot be changed, indicate a pessimistic fixpoint and give up.

[Attributor] Revision for AAAllocationInfo attribute based on Review

jdoerfert retitled this revision from New attribute to identify what byte ranges are alive for an allocation to [Attributor] New attribute to identify what byte ranges are alive for an allocation.Aug 15 2023, 12:54 PM
jdoerfert added reviewers: tianshilei1992, jhuber6.
jdoerfert removed a project: Restricted Project.
Herald added a project: Restricted Project. · View Herald Transcript
vidsinghal published this revision for review.Aug 15 2023, 1:02 PM
Herald added a project: Restricted Project. · View Herald Transcript
jdoerfert added inline comments.Aug 15 2023, 1:13 PM
llvm/lib/Transforms/IPO/Attributor.cpp
3646–3648
llvm/lib/Transforms/IPO/AttributorAttributes.cpp
885

Call it numBins or sth, to make it clear. Size could be a lot of things in this context.

12674

no this->

12676

This is just getIRPosition().

12684

Why would we want to have the same value twice, use the pointer.

12686–12688

Style, also elsewhere.

12690

We should not cast things to the Impl. Expose the functions you need in the AAPointerInfo interface instead.

12706

I think A.getDataLayout() is a thing too.

12731–12733

Make this an assertion, manifest will not be called on invalid states.

vidsinghal marked 8 inline comments as done.
vidsinghal edited the summary of this revision. (Show Details)

Fixed some comments.

vidsinghal added inline comments.Aug 20 2023, 7:00 AM
llvm/lib/Transforms/IPO/AttributorAttributes.cpp
12690

I think it might make sense to expose the state in the AAPointerInfo struct.
AAAllocation Info might need more information from AAPointerInfo for more complex cases.
But I wanted to get an opinion before doing that.
Right now, State is defined in the AAPointerInfo namespace in llvm/lib/Transforms/IPO/AttributorAttributes.cpp
However, the AAPointerInfo struct does not store it, so that information cannot be accessed without casting to AAPointerInfoImpl.

Edited unit tests

Fix typo in assert.
Add a positive test case for malloc with a TODO.

This comment was removed by vidsinghal.
jdoerfert added inline comments.Aug 20 2023, 3:11 PM
llvm/lib/Transforms/IPO/AttributorAttributes.cpp
12686
12694

Don't cast it to an "Impl". There is no need anymore anyway.

12695

State could be anything. At least call it PIState.

12699–12705

This is basically BinSize != 1, return, no?

12718

Why does this not trigger for the ptr/i64 case? Is one of them in bytes the other in bits?

12721

This is fine for OffsetEnd = 2,4,8, but then it becomes a little iffy. Check for non default sizes and go with a char array instead.

12739

Same as above.

llvm/test/Transforms/Attributor/value-simplify-local-remote.ll
82 ↗(On Diff #551856)

This doesn't actually make it smaller, we should avoid such changes.

135 ↗(On Diff #551856)

Same here.

vidsinghal marked 13 inline comments as done.

Revision based on comments

llvm/lib/Transforms/IPO/AttributorAttributes.cpp
12699–12705

umm, I think BinSize == 0

12718

if its a ptr

public: std::optional<TypeSize> getAllocationSize(const DataLayout &DL)

returns a size of 0 which is why that if condition is not triggered.
At the LLVM IR level, I think the size of pointer is just 0.

jdoerfert added inline comments.Aug 21 2023, 6:04 PM
llvm/lib/Transforms/IPO/AttributorAttributes.cpp
12727

Why do you give up on struct type like {i32, i32}?
I don't understand why we need this conditional at all.

vidsinghal marked an inline comment as done.

Remove un-necessary if condition for struct with only one element.

vidsinghal added inline comments.Aug 21 2023, 6:50 PM
llvm/lib/Transforms/IPO/AttributorAttributes.cpp
12727

I was trying to indicate a fix point for structs like
type S = {ptr}
where the struct has only one element in it.
But I think that OffsetEnd == AllocationSize check should work for this now (after fixing the size of pointer issue) I'll remove this check.
Previously this was not working which is why I had to had this check and i think
getNumContainedTypes should have been getStructNumElements instead.

check for zero alloca and indicate fixpoint.

tianshilei1992 added inline comments.Aug 23 2023, 2:17 PM
llvm/lib/Transforms/IPO/Attributor.cpp
3653

Better to use [[maybe_unused]] now.

jdoerfert added inline comments.Aug 23 2023, 2:59 PM
llvm/lib/Transforms/IPO/AttributorAttributes.cpp
12689–12690

As I said before, no casting to implementations of the interface. put the method you need into AAPointerInfo.

12738–12740
llvm/test/Transforms/Attributor/value-simplify-local-remote.ll
384 ↗(On Diff #552461)

unrelated, just keep the old version of this file.

vidsinghal marked an inline comment as done.

1.) Insert a new instruction instead of modiying the old one (Char array)
2.) Restore value-simplify-local-remote.ll
3.) Add a new positive test to reduce size of array alloca

vidsinghal marked 2 inline comments as done.

1.) Put method to get offsetBins in AAPointerInfo
2.) work on other comments.

vidsinghal marked an inline comment as done.

add maybe_unused pragma

jdoerfert added inline comments.Aug 27 2023, 11:05 AM
llvm/include/llvm/Transforms/IPO/Attributor.h
6127

This is not a good idea. What happend to the old API you had?
We do not want to leak out the underlying map if we don't have to.
All you want is a size and begin/end, no?

vidsinghal added inline comments.Aug 27 2023, 11:33 AM
llvm/include/llvm/Transforms/IPO/Attributor.h
6127

Yeah I see.
The underlying map is protected in the state I think.
Yeah for now, size begin and end should be good.
I think I will try making those virtual pure in AAPointerInfo and then override them in the state?

Add API for begin, end size in AAPointerInfo

vidsinghal marked an inline comment as done.Aug 27 2023, 11:59 AM

Add a positive test with TODO

vidsinghal updated this revision to Diff 555543.Sep 1 2023, 6:50 PM

Make switch to indentify alloca vs malloc instruction

tianshilei1992 added inline comments.Sep 1 2023, 6:51 PM
llvm/lib/Transforms/IPO/Attributor.cpp
3653

Then there is no need (void)Success;.

vidsinghal updated this revision to Diff 555544.Sep 1 2023, 7:03 PM

remove (void)success

vidsinghal marked an inline comment as done.Sep 1 2023, 7:04 PM
vidsinghal updated this revision to Diff 555601.Sep 2 2023, 2:19 PM

Support for modifying malloc instructions
TODO: The original malloc call is not being removed by changeAfterManifest even though it is dead.

vidsinghal updated this revision to Diff 555605.Sep 2 2023, 4:23 PM

fix name variable

vidsinghal updated this revision to Diff 555607.Sep 2 2023, 5:59 PM

Delete malloc call after attributor update

vidsinghal updated this revision to Diff 555608.Sep 2 2023, 6:14 PM

fix example IR

vidsinghal updated this revision to Diff 555610.Sep 2 2023, 8:51 PM

Don't call AAPointerInfoGetOrCreate other than Alloca and Malloc Instruction. Results in infinite loop for instance when calling a ptr

vidsinghal updated this revision to Diff 555633.Sep 3 2023, 9:03 AM

Clean up code

Add new test where argument to malloc isn't a constant.
Use AAPotentialConstantValues to get the constant value.

Add null check for IntOperand

vidsinghal updated this revision to Diff 555765.Sep 4 2023, 9:42 AM

New example and question about what the optimization should do for an example like @baz in allocator.ll

Remove run commands in depgraph.ll that were not being parsed