This is an archive of the discontinued LLVM Phabricator instance.

[Attributor] Guarantee getAAFor not to update AA in the manifestation stage
ClosedPublic

Authored by okura on Aug 26 2020, 9:09 AM.

Details

Summary

If we query an AA with Attributor::getAAFor in AbstractAttribute::manifest, the AA may be updated.
This patch makes use of the phase flag in Attributor, and handle getAAFor behavior according to the flag.

Diff Detail

Event Timeline

okura created this revision.Aug 26 2020, 9:09 AM
Herald added a reviewer: homerdin. · View Herald Transcript
Herald added a project: Restricted Project. · View Herald Transcript
okura requested review of this revision.Aug 26 2020, 9:09 AM

Can you replace all uses of getAAFor in manifest by the new API in this patch?

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

Should we call this "getAADuringManifest" or do we expect another place we use it?

okura added inline comments.Aug 26 2020, 10:01 AM
llvm/include/llvm/Transforms/IPO/Attributor.h
979

I will rename this getAADuringManifestFor.

okura updated this revision to Diff 288036.Aug 26 2020, 10:40 AM
  • rename getter getAADuringManifestFor
  • replace some existing getAAFor in manifest with this

Some AAs use checkForAll... in manifest, and getAAFor is used in the function.
I think we need to add a new parameter like bool DuringManifest to them in a follow-up.

llvm/lib/Transforms/IPO/AttributorAttributes.cpp
3070 ↗(On Diff #288036)

Though I'm not sure why we force update and track dependence here, I think we can replace this with a new API.

I think rather than creating a new api function, it would be more robust if we had a flag as member of the attributor.
It would be similar to SeedingPeriod flag that I have added.

Simply we would make it true in the Attributor::manifestAttributes function and then we would have a assertion in Attributor::updateAA

I think rather than creating a new api function, it would be more robust if we had a flag as member of the attributor.
It would be similar to SeedingPeriod flag that I have added.

Simply we would make it true in the Attributor::manifestAttributes function and then we would have a assertion in Attributor::updateAA

Thank you for the suggestion. I'm thinking about the same approach. I totally agree with @kuter. It is slightly hard for us to guarantee Attributor not to update AAs during manifestation by adding a new API.
Adding a flag that indicates which stage of the process (e.g. the update stage or the manifestation stage) we are in is better, I think.
@jdoerfert How do you think about this?

Yeah, I'm fine with a member flag. Though we should consider making the thing an enum now, like

enum {
  SEEDING,
  UPDATE,
  MANIFEST,
} Phase;

or something like that. Once we have to juggle 2 booleans this might already by easier.

okura updated this revision to Diff 288381.Aug 27 2020, 10:14 AM
  • make use of the phase flag
okura retitled this revision from [Attributor] Add getter of abstract attributes for the manifestation stage to [Attributor] Guarantee getAAFor not to update AA in the manifestation stage.Aug 27 2020, 10:18 AM
okura edited the summary of this revision. (Show Details)
okura added inline comments.Aug 27 2020, 10:33 AM
llvm/include/llvm/Transforms/IPO/Attributor.h
983

Some AA uses the same helper function that contains getAndUpdateAAFor in both updateImpl and manifest. Therefore, we should ignore the enforcement in this case rather than abort.
Though I want to use a debug printer here for this reason, #define DEBUG_TYPE "attributor" is not defined in Attributor.h.
If there is no reason to put the implementation Attributor.h, I want to move the implementation into Attributor.cpp.

jdoerfert accepted this revision.Aug 27 2020, 10:53 AM

LGTM, without print, if you want the print we can talk about it.

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

I want to move the implementation into Attributor.cpp.

That doesn't work because we need to specialize the template (I think).

I'd go without a print for now.

This revision is now accepted and ready to land.Aug 27 2020, 10:53 AM
okura added inline comments.Aug 27 2020, 11:00 AM
llvm/include/llvm/Transforms/IPO/Attributor.h
983

I understand. I will delete debug print.

okura updated this revision to Diff 288392.Aug 27 2020, 11:05 AM
  • delete debug print
  • add a comment