This is an archive of the discontinued LLVM Phabricator instance.

[Attributor] Introduce module slice.
ClosedPublic

Authored by sstefan1 on Aug 20 2020, 3:25 PM.

Details

Summary

The module slice describes which functions we can analyze and transform
while working on an SCC as part of the Attributor-CGSCC pass. So far we
simply restricted it to the SCC.

Diff Detail

Event Timeline

sstefan1 created this revision.Aug 20 2020, 3:25 PM
Herald added a reviewer: homerdin. · View Herald Transcript
Herald added a project: Restricted Project. · View Herald Transcript
sstefan1 requested review of this revision.Aug 20 2020, 3:25 PM

I went to some tests and left some comments. tomorrow we go through the rest.

llvm/test/Transforms/Attributor/ArgumentPromotion/X86/attributes.ll
1

Please pre-commit the additional flags and updates to the tests as NFC (no review)

llvm/test/Transforms/Attributor/ArgumentPromotion/alignment.ll
19

This is the expected result, the CGSCC and module pass are producing the same in NPM and OPM respectively.

122

It's really important that argument privatization works now in the CGSCC pass! yay!

llvm/test/Transforms/Attributor/IPConstantProp/musttail-call.ll
92–95

This looks wrong though. Need to verify.

kuter added a comment.Aug 20 2020, 6:39 PM

This is interesting, I thought you couldn't change the IR outside of the current SCC.
Was this intended to go on top of D86081 ?

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

I don't think this is needed. If you just seed the SCC, the AAs won't query anything outside of this anyways.

1085–1090

Documentation

llvm/lib/Transforms/IPO/AttributorAttributes.cpp
3035–3042

I think this to not spend too much time on functions that are outside of the current SCC right ?
Can you add comments.

This is interesting, I thought you couldn't change the IR outside of the current SCC.

We can't delete functions outside of the SCC, the rest should be fine.

Was this intended to go on top of D86081 ?

At first yes. However we need to have manifest in module slice since we need to manifest stuff (if any) and we don't allow deleting functions outside of the SCC. You will probably need to make that allow manifest only on the module slice (but probably unlikely it will ever happen outside of it).

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

While that is right, module slice won't be used by the attributor exclusively, that's why it is in the InformationCache. We'll use this in the OpenMPOpt as well.

llvm/lib/Transforms/IPO/AttributorAttributes.cpp
3035–3042

Yes, we don't want to compute it twice.
Will update, this was just a first draft.

okura added inline comments.Aug 22 2020, 5:31 AM
llvm/test/Transforms/Attributor/IPConstantProp/musttail-call.ll
92–95

This is because of AANoUndef implementation. I have made a patch for this. D86396

sstefan1 updated this revision to Diff 288608.Aug 28 2020, 7:46 AM

Rebasing after recent changes to AANoUndef

llvm/test/Transforms/Attributor/IPConstantProp/musttail-call.ll
92–95

Thanks again for taking a look!

jdoerfert accepted this revision.Aug 29 2020, 2:15 PM

LGTM, one nit

llvm/include/llvm/Transforms/IPO/Attributor.h
1092–1095

make this a proper query method, isInModuleSlice or something.

This revision is now accepted and ready to land.Aug 29 2020, 2:15 PM
This revision was automatically updated to reflect the committed changes.