This is an archive of the discontinued LLVM Phabricator instance.

[Attributor] Run lightweight version of the Attributor by default.
Needs ReviewPublic

Authored by sstefan1 on Apr 12 2021, 12:55 PM.

Details

Summary

Restrict Attributor to a subset of AbstractAttributes by default.
Added a flag to enable full attributor run.

Diff Detail

Event Timeline

sstefan1 created this revision.Apr 12 2021, 12:55 PM
sstefan1 requested review of this revision.Apr 12 2021, 12:55 PM
Herald added a project: Restricted Project. · View Herald Transcript

Cool, thanks! I'll give it a swirl can you address the nit below before?

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

The last two are rather costly. We should remove them for now.

Interesting. Do we have near term plans for adding the Attributor to the default pass pipeline ?

sstefan1 updated this revision to Diff 337103.Apr 13 2021, 4:51 AM

Removing AAMemory*

Interesting. Do we have near term plans for adding the Attributor to the default pass pipeline ?

That's the plan. We'll see how it goes.

xbolva00 added subscribers: nikic, xbolva00.EditedApr 13 2021, 5:22 AM

Interesting. Do we have near term plans for adding the Attributor to the default pass pipeline ?

That's the plan. We'll see how it goes.

Consider using @nikic's llvm compile time tracker for this patch and/or for full enablement of attributor.

http://llvm-compile-time-tracker.com/about.php

madhur13490 added inline comments.
llvm/lib/Transforms/IPO/Attributor.cpp
2338

May I know the rationale behind this list? Is it result of some empirical analysis?

madhur13490 added inline comments.Apr 13 2021, 6:25 AM
llvm/lib/Transforms/IPO/Attributor.cpp
2338

Is there any plan to allow backends to customize this list?

jdoerfert added inline comments.Apr 13 2021, 8:22 AM
llvm/lib/Transforms/IPO/Attributor.cpp
2338

These are relatively simple to compute attributes that pretty much match the FunctAttrs pass. It's an initial list, not set in stone.

Customization hooks are welcome as patches once something lands :)

Isn't the PassManagerBuilder part of this patch missing to actually enable the lightweight Attributor by default?

Isn't the PassManagerBuilder part of this patch missing to actually enable the lightweight Attributor by default?

I thought we should flip the switch in a separate patch. We would only need to make -enable-attributor=all (or some other configuration) the default.

Interesting. Do we have near term plans for adding the Attributor to the default pass pipeline ?

That's the plan. We'll see how it goes.

Consider using @nikic's llvm compile time tracker for this patch and/or for full enablement of attributor.

http://llvm-compile-time-tracker.com/about.php

Will do that!

High level meta comment on enabling attributor by default, not on the code per se.

I think this deserves to be raised and discussed on llvm-dev. You are essentially proposing that we adopt Attributor as our long term plan for IPO in LLVM. This has not been widely discussed and agreed upon. You need to send an RFC before enabling this by default.

I'm about to list a couple of concerns I have. Please don't debate me here. Instead, consider this advanced warning of the issues likely to come up in an llvm-dev thread, and have your data and arguments ready. (i.e. These are probably concerns you want to address in your RFC.)

One major concern I have is the implications for compile time. My understanding (which may be wrong) is that Attributor iterates to a fixed point between multiple attributes over the entire call graph. The existing inference code intentionally and specifically does not do the same. Fixed point iteration is potentially much more expensive then the existing post order walk. I think this needs to be measured, and discussed explicitly on llvm-dev. I could see an argument that we may be willing to accept the increased compile time at O3, but I am skeptical that this will be worthwhile at O2 and below. You need to make that argument, on llvm-dev, with evidence.

A second major concern I have is correctness. To my knowledge, Attributor has not been extensively fuzzed or otherwise programmatically tested at scale. Before this enabled (not before a flag is added), you need to do the work and/or make the argument that you have done the work, to justify a reasonable belief that Attributor is in fact as correct as the code it is replacing/supplementing.

I just want to mention that this is just a starting point, as suggested in D99769. It is definitely not our attention to rush and do this without an RFC or numbers that are in favour of the switch.

The main reason for this 'lightweight' version is exactly the compile time and I will definitely measure it. This only introduces the lightweight version, so that it will be easier to switch, when we come to that point.

I hope we are on the same page now.

I hope we are on the same page now.

Provided the RFC happens *before* any version of attributor is enabled by default, yes, we are.

fhahn added a comment.Apr 15 2021, 9:08 AM

I just want to mention that this is just a starting point, as suggested in D99769. It is definitely not our attention to rush and do this without an RFC or numbers that are in favour of the switch.

The main reason for this 'lightweight' version is exactly the compile time and I will definitely measure it. This only introduces the lightweight version, so that it will be easier to switch, when we come to that point.

I hope we are on the same page now.

Thanks for putting up the patch to get things started! I'd recommend trying to build https://github.com/llvm/llvm-test-suite in various configurations and investigating the crashes. For example, I just tried to do so with AttributorRunOption::ALL with -O3 -flto on X86 and there's a crash building MultiSource/Applications/lambda-0.1.3.

Thanks for putting up the patch to get things started! I'd recommend trying to build https://github.com/llvm/llvm-test-suite in various configurations and investigating the crashes. For example, I just tried to do so with AttributorRunOption::ALL with -O3 -flto on X86 and there's a crash building MultiSource/Applications/lambda-0.1.3.

I started to do this as well now. I reported the non-Attributor bug here: https://bugs.llvm.org/show_bug.cgi?id=49984
Any help identifying and categorizing issues would be very much appreciated :)

reames resigned from this revision.Nov 30 2021, 9:58 AM
ormris removed a subscriber: ormris.Jan 24 2022, 11:14 AM