Page MenuHomePhabricator

[Attributor] Adds deduction for the MaxObjSize Attribute
Needs ReviewPublic

Authored by atmnpatel on Sep 19 2020, 3:55 PM.

Details

Summary

This patch adds deduction for the MaxObjSize attribute in the
Attributor. If an object is globally defined or has been defined with an
alloca instruction, it's not resizable and we immediately converge to
the size. For objects that are captured, we assume the pessimistic
fixpoint because the objects may be resized, and we iterate for the
rest.

Diff Detail

Unit TestsFailed

TimeTest
160 mslinux > LLVM.Transforms/Attributor/ArgumentPromotion::sret.ll
Script: -- : 'RUN: at line 2'; /mnt/disks/ssd0/agent/llvm-project/build/bin/opt -attributor -enable-new-pm=0 -attributor-manifest-internal -attributor-max-iterations-verify -attributor-annotate-decl-cs -attributor-max-iterations=4 -S < /mnt/disks/ssd0/agent/llvm-project/llvm/test/Transforms/Attributor/ArgumentPromotion/sret.ll | /mnt/disks/ssd0/agent/llvm-project/build/bin/FileCheck /mnt/disks/ssd0/agent/llvm-project/llvm/test/Transforms/Attributor/ArgumentPromotion/sret.ll --check-prefixes=CHECK,NOT_CGSCC_NPM,NOT_CGSCC_OPM,NOT_TUNIT_NPM,IS__TUNIT____,IS________OPM,IS__TUNIT_OPM

Event Timeline

atmnpatel created this revision.Sep 19 2020, 3:55 PM
atmnpatel requested review of this revision.Sep 19 2020, 3:55 PM
jdoerfert added inline comments.Sep 21 2020, 3:19 PM
llvm/lib/Transforms/IPO/AttributorAttributes.cpp
3712–3713

Make it StateType

3741
3742

assert(T.isAtFixpoint() && "...");

3744

Resizable should be taken from AA not from nocapture. Nocapture is needed only if resizable is true. So if it is resizable and it maybe captured. Give up on it.

Addresses inline comments (I think), and adds a test.

Decreased bloat in the OpenMP test modifications.

Clang-tidy fix.

jdoerfert added inline comments.Sep 28 2020, 12:05 AM
llvm/test/Transforms/Attributor/maxobjsize.ll
174

add another use of %0 and @G here before the capture calls

174

This is not necessary, forgot to delete it.

190

duplicate the test so it exists 3 times. In this version we can actually add maxobjsize to the call site argument, we can do so as well if the use is not reachable from a capturing use. We don't need to add the logic but the test might be worth adding with a fixme.

jdoerfert added inline comments.Sep 29 2020, 11:15 AM
llvm/lib/Transforms/IPO/AttributorAttributes.cpp
3839

This can be null. We need to check it and abort.

atmnpatel retitled this revision from [WIP][Attributor] Adds deduction for the MaxObjSize Attribute to [Attributor] Adds deduction for the MaxObjSize Attribute.Sep 29 2020, 11:24 AM
This comment was removed by jdoerfert.
jdoerfert added inline comments.Sep 29 2020, 11:51 AM
llvm/lib/Transforms/IPO/AttributorAttributes.cpp
3793

you need a new statistics tracker here and elsewhere

changed stats trackers

Fixed broken test.

jdoerfert added a comment.EditedOct 1 2020, 6:16 PM

CTMark, O3 with attributor before and after these patches

0) before_attr.json.stats vs after_attr.json.stats
CHANGED: codegenprepare               NumExtsMoved                                         3631 ->       3699 (    +1.873%)
CHANGED: dse                          NumFastOther                                          192 ->        194 (    +1.042%)
CHANGED: gvn                          IsValueFullyAvailableInBlockNumSpeculationsMax       4958 ->       5060 (    +2.057%)
CHANGED: gvn                          NumGVNInstr                                         46657 ->      47534 (    +1.880%)
CHANGED: jump-threading               NumDupes                                               91 ->         92 (    +1.099%)
CHANGED: licm                         NumMovedLoads                                        6272 ->       6344 (    +1.148%)
CHANGED: licm                         NumPromoted                                           381 ->        438 (   +14.961%)
CHANGED: loop-rotate                  NumNotRotatedDueToHeaderSize                           31 ->         29 (    -6.452%)
CHANGED: machinelicm                  NumPostRAHoisted                                       88 ->         89 (    +1.136%)
CHANGED: memdep                       NumCacheNonLocalPtr                               1005887 ->    1016671 (    +1.072%)
CHANGED: memory-builtins              ObjectVisitorLoad                                   62048 ->      63473 (    +2.297%)
CHANGED: peephole-opt                 NumCmps                                               532 ->        526 (    -1.128%)
CHANGED: regalloc                     NumDCEFoldedLoads                                      27 ->         26 (    -3.704%)
CHANGED: regalloc                     NumLocalSplits                                       1891 ->       1870 (    -1.111%)
jdoerfert added a comment.EditedOct 1 2020, 6:22 PM

Can you add these examples too please:

void use(int *a) {
  *a += 1;
}
static void non_resizable(int *p) {
  use(p);
  use(p);
}
static void resizable(int *p) {
  use(p);
  use(p);
}
void non_resizable_caller1() {
  int a;
  non_resizable(&a);
}
int G;
void non_resizable_caller2() {
  non_resizable(&G);
}
void resizable_caller1() {
  int a;
  resizable(&a);
}
void resizable_caller2(int *a) {
  resizable(a);
}
llvm/lib/Transforms/IPO/AttributorAttributes.cpp
3742

Why not the assumed state?

jdoerfert added inline comments.Oct 14 2020, 12:21 PM
llvm/test/Transforms/Attributor/maxobjsize.ll
494

Can you run mem2reg on the new test code. Also add maxobjsize to the argument %0 in this caller, I forgot to mention that. The test should show that we propagate maxobjsize to the second use *only* if it is not resizable.