Page MenuHomePhabricator

[Attributor] Introduce CallBaseContext to the IRPosition
AcceptedPublic

Authored by kuter on Jun 16 2020, 5:04 AM.

Details

Summary

This patch introduces call base context information to the IRPosition
This patch is part of the patch set that are going to introduce call base specific analysis.

Diff Detail

Unit TestsFailed

TimeTest
9,840 mslinux > libomp.env::Unknown Unit Message ("")
Script: -- : 'RUN: at line 1'; /mnt/disks/ssd0/agent/llvm-project/build/./bin/clang -fopenmp -pthread -fno-experimental-isel -I /mnt/disks/ssd0/agent/llvm-project/openmp/runtime/test -I /mnt/disks/ssd0/agent/llvm-project/build/projects/openmp/runtime/src -L /mnt/disks/ssd0/agent/llvm-project/build/lib -I /mnt/disks/ssd0/agent/llvm-project/openmp/runtime/test/ompt /mnt/disks/ssd0/agent/llvm-project/openmp/runtime/test/env/kmp_set_dispatch_buf.c -o /mnt/disks/ssd0/agent/llvm-project/build/projects/openmp/runtime/test/env/Output/kmp_set_dispatch_buf.c.tmp -lm -latomic
1,590 mslinux > libomp.worksharing/for::Unknown Unit Message ("")
Script: -- : 'RUN: at line 1'; /mnt/disks/ssd0/agent/llvm-project/build/./bin/clang -fopenmp -pthread -fno-experimental-isel -I /mnt/disks/ssd0/agent/llvm-project/openmp/runtime/test -I /mnt/disks/ssd0/agent/llvm-project/build/projects/openmp/runtime/src -L /mnt/disks/ssd0/agent/llvm-project/build/lib -I /mnt/disks/ssd0/agent/llvm-project/openmp/runtime/test/ompt /mnt/disks/ssd0/agent/llvm-project/openmp/runtime/test/worksharing/for/kmp_set_dispatch_buf.c -o /mnt/disks/ssd0/agent/llvm-project/build/projects/openmp/runtime/test/worksharing/for/Output/kmp_set_dispatch_buf.c.tmp -lm -latomic && /mnt/disks/ssd0/agent/llvm-project/build/projects/openmp/runtime/test/worksharing/for/Output/kmp_set_dispatch_buf.c.tmp 7

Event Timeline

kuter created this revision.Jun 16 2020, 5:04 AM
Herald added a reviewer: sstefan1. · View Herald Transcript
Herald added a reviewer: uenoku. · View Herald Transcript
Herald added a project: Restricted Project. · View Herald Transcript
kuter added a comment.Jun 16 2020, 5:06 AM

TODO: comments

We need some way of testing this, so tests that enable this and some way of checking the output. We could look for the debug output or add some other way of checking

We need some way of testing this, so tests that enable this and some way of checking the output. We could look for the debug output or add some other way of checking

Reverse ping. Unit tests might also be an option.

xbolva00 added inline comments.
llvm/lib/Transforms/IPO/Attributor.cpp
83

typo

kuter added a comment.Jul 2 2020, 2:48 PM

We need some way of testing this, so tests that enable this and some way of checking the output. We could look for the debug output or add some other way of checking

Reverse ping. Unit tests might also be an option.

Wouldn't just testing the uses of this path enough.
For example if we have are getting a correct call site constant range would that be enough ?

We need some way of testing this, so tests that enable this and some way of checking the output. We could look for the debug output or add some other way of checking

Reverse ping. Unit tests might also be an option.

Wouldn't just testing the uses of this path enough.
For example if we have are getting a correct call site constant range would that be enough ?

Is the CBContext already used? I might have to take another look at this, sorry.

kuter added a comment.Jul 2 2020, 5:25 PM

We need some way of testing this, so tests that enable this and some way of checking the output. We could look for the debug output or add some other way of checking

Reverse ping. Unit tests might also be an option.

Wouldn't just testing the uses of this path enough.
For example if we have are getting a correct call site constant range would that be enough ?

Is the CBContext already used? I might have to take another look at this, sorry.

Not by this patch no. But I have patches that use CBContext(That I am going to send to phabricator soon).

kuter updated this revision to Diff 275285.Jul 2 2020, 8:50 PM

general bug fix.

kuter added a comment.EditedJul 2 2020, 9:02 PM

Btw this patch introduces 8 bytes per attribute because of the CBContext.
I think we would ideally move it to a map inside the attributor.

I can inject the context through the update method or make the attribute ask the Attributor
like A.getCBContext(this); one problem with it is that since the operator<< for IRPosition don't have access
to the attributor we would not be able to print it even though it is useful to have.

One potential solution to this is to use a macro switch to have the CBContext inside the IRPosition only in debug builds.

Any other ideas ?

kuter updated this revision to Diff 277674.Mon, Jul 13, 10:42 PM

Fix typo

kuter marked an inline comment as done.Mon, Jul 13, 10:42 PM

Btw this patch introduces 8 bytes per attribute because of the CBContext.

[...]

Any other ideas ?

Run heapcheck and if the cgscc memory usage is not impacted much keep it this way ;)

We need a test for this. We can use unit tests or check the printed output (lit test + requires: asserts IIRC). Either works but we should add one.

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

I'd prefer to copy the entire thing and then null the context. Will be optimized to the same code but is future proof :)

462

Documentation for all of them please.

591

Nit: I would prefer to move/add the initialization here. Assuming it is all the same to you.

953

Move this into a helper function so we don't have to "make LLVM_DEBUG" work here.

955

& RealIRP or maybe:

if (!should...)
  IRP = IRP.strip...
llvm/lib/Transforms/IPO/Attributor.cpp
91

Nit: attributor-enable-call-site-specific is missing a final word, maybe specialization or deduction or something.

466

Good thinking!

804

Nit, formatting.

kuter marked 2 inline comments as done.Fri, Jul 17, 8:19 PM
kuter added inline comments.
llvm/include/llvm/Transforms/IPO/Attributor.h
457

Ugh sorry. I will.

955

I think that was the first way that I tried.
Reassigning a const reference does not work.

jdoerfert added inline comments.Fri, Jul 17, 8:37 PM
llvm/include/llvm/Transforms/IPO/Attributor.h
955

You are right. We can remove the const and reference from IRP actually. Either way is fine with me. If we do it as you have it now, maybe make it a reference. I'm not sold on Real as a name, maybe Effective, but that is not perfect either.

kuter updated this revision to Diff 279049.Sat, Jul 18, 9:05 PM

add missing documentation.
improve formating and readability.
fix typos.
rename command line option

kuter marked 5 inline comments as done.Sat, Jul 18, 9:07 PM
kuter marked 2 inline comments as done.

All this is missing now is a test :). We can just check for the debug output.

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

Nit: newline

Now we are in a hybrid solution, should we do if (should...) IRP = ... ?

kuter added a comment.Sun, Jul 19, 5:32 PM

All this is missing now is a test :). We can just check for the debug output.

The problem I have is that this patch does not add anything that would trigger any of the code here.
There isn't much logic here either, most of it is in D83744.
I can add a unittest for 'hasCallBaseContext` and stripCallBaseContext

Hmm, I hoped with the flag you would see the context printed somewhere but it is never added I suppose. Do we have a unit test setup for the Attributor? Would be good :D

kuter added a comment.Mon, Jul 20, 5:28 PM

Memory usage for this patch:
comand: $LLVM_BIN/opt -S -attributor-cgscc test-suit/CTMark/SPASS/clause.ll
before:

calls to allocation functions: 78794 (82506/s)
temporary memory allocations: 3942 (4127/s)
peak heap memory consumption: 2.58MB
peak RSS (including heaptrack overhead): 355.73MB
total memory leaked: 140.33KB

after:

calls to allocation functions: 78802 (86977/s)
temporary memory allocations: 3942 (4350/s)
peak heap memory consumption: 2.59MB
peak RSS (including heaptrack overhead): 356.60MB
total memory leaked: 140.33KB

difference:

calls to allocation functions: 8 (-163/s)
temporary memory allocations: 0 (0/s)
peak heap memory consumption: 8.26KB
peak RSS (including heaptrack overhead): 0B
total memory leaked: 0B

Interesting, there doesn't seem to be much of a difference.

kuter updated this revision to Diff 279421.EditedMon, Jul 20, 9:32 PM

Add unittest.

kuter retitled this revision from [WIP][Attributor] Introduce CallBaseContext to the IRPosition to [Attributor] Introduce CallBaseContext to the IRPosition.Mon, Jul 20, 9:33 PM
fhahn added a subscriber: fhahn.Tue, Jul 21, 2:13 AM
kuter updated this revision to Diff 280302.Thu, Jul 23, 5:21 PM

fix styling.

jdoerfert accepted this revision.Fri, Jul 24, 7:41 AM

LGTM with nits.

llvm/include/llvm/Transforms/IPO/Attributor.h
453–467

Nit: If this is not yet upstream, use the proper R from the beginning.

915

Not needed anymore?

937

my bad, if (!should..), right?

997

Not needed anymore.

This revision is now accepted and ready to land.Fri, Jul 24, 7:41 AM
kuter marked an inline comment as done.EditedFri, Jul 24, 4:37 PM

LGTM with nits.

I made a mistake while creating this revision.
This is incomplete. instead of amending the [Attributor] Introduce CallBaseContext to the IRPosition I created a new commit and when I did arc diff It updated it to the state it is.
I will update this to include all of the patch.

sorry for the mess.

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

Yes thank you, I should have ran the tests in D83744 before updating the revision.

kuter updated this revision to Diff 280650.Fri, Jul 24, 6:48 PM

add missing code.
remove unneeded definies.
fix bug.