Restrict Attributor to a subset of AbstractAttributes by default.
Added a flag to enable full attributor run.
|280 ms||x64 debian > LLVM.tools/UpdateTestChecks/update_test_checks::check_attrs.test|
Script: -- : 'RUN: at line 2'; cp -f /mnt/disks/ssd0/agent/llvm-project/llvm/test/tools/UpdateTestChecks/update_test_checks/Inputs/check_attrs.ll /mnt/disks/ssd0/agent/llvm-project/build/test/tools/UpdateTestChecks/update_test_checks/Output/check_attrs.test.tmp.ll && '/usr/bin/python3.8' /mnt/disks/ssd0/agent/llvm-project/build/test/../../llvm/utils/update_test_checks.py --opt-binary /mnt/disks/ssd0/agent/llvm-project/build/test/../bin/opt /mnt/disks/ssd0/agent/llvm-project/build/test/tools/UpdateTestChecks/update_test_checks/Output/check_attrs.test.tmp.ll --function-signature
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 :)
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.
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.