Restrict Attributor to a subset of AbstractAttributes by default.
Added a flag to enable full attributor run.
Details
Diff Detail
- Repository
- rG LLVM Github Monorepo
Unit Tests
Event Timeline
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 ?
Consider using @nikic's llvm compile time tracker for this patch and/or for full enablement of attributor.
llvm/lib/Transforms/IPO/Attributor.cpp | ||
---|---|---|
2338 | May I know the rationale behind this list? Is it result of some empirical analysis? |
llvm/lib/Transforms/IPO/Attributor.cpp | ||
---|---|---|
2338 | Is there any plan to allow backends to customize this list? |
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?
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.
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.
Provided the RFC happens *before* any version of attributor is enabled by default, yes, we are.
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 :)
May I know the rationale behind this list? Is it result of some empirical analysis?