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

[okura]

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.

[jdoerfert]

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]

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

979

I will rename this getAADuringManifestFor.

[okura]

• rename getter getAADuringManifestFor
• replace some existing getAAFor in manifest with this

[okura]

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.

[kuter]

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

[okura]

In D86635#2239738, @kuter wrote:

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?

[jdoerfert]

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]

• make use of the phase flag

[okura]

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]

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.

[okura]

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

983

I understand. I will delete debug print.

[okura]

• delete debug print
• add a comment

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

struct Attributor {
   /// only the Attributor itself. Initial seeding of AAs can be done via this
   /// function.
   template <typename AAType>
   const AAType &getOrCreateAAFor(const IRPosition &IRP,
                                  const AbstractAttribute *QueryingAA = nullptr,
                                  bool TrackDependence = false,
                                  DepClassTy DepClass = DepClassTy::OPTIONAL,
                                  bool ForceUpdate = false) {
+#define DEBUG_TYPE "attributor"
     if (AAType *AAPtr = lookupAAFor<AAType>(IRP, QueryingAA, TrackDependence)) {
-      if (ForceUpdate)
+      if (ForceUpdate) {
+        if (Phase != AttributorPhase::UPDATE)
+          LLVM_DEBUG(dbgs() << "[Attributor] An update is forced outside of "
+                               "the update stage. Ignore this enforcement.\n");
+        else
           updateAA(*AAPtr);
+      }
       return *AAPtr;
     }
+#undef DEBUG_TYPE
 
     // No matching attribute found, create one.
     // Use the static create method.
     auto &AA = AAType::createForPosition(IRP, *this);
 
     // If we are currenty seeding attributes, enforce seeding rules.
     if (Phase == AttributorPhase::SEEDING && !shouldSeedAttribute(AA)) {
       AA.getState().indicatePessimisticFixpoint();
#undef DEBUG_TYPE
     // We can initialize (=look at) code outside the current function set but
     // not call update because that would again spawn new abstract attributes in
     // potentially unconnected code regions (=SCCs).
     if (FnScope && !Functions.count(const_cast<Function *>(FnScope))) {
       AA.getState().indicatePessimisticFixpoint();
       return AA;
     }
 
+    // If this is queried in the manifest stage, we force the AA to indicate
+    // pessimistic fixpoint immediately.
+    if (Phase == AttributorPhase::MANIFEST) {
+      AA.getState().indicatePessimisticFixpoint();
+      return AA;
+    }
+
     // Allow seeded attributes to declare dependencies.
     // Remember the seeding state.
     AttributorPhase OldPhase = Phase;
     Phase = AttributorPhase::UPDATE;
 
     updateAA(AA);
 
     Phase = OldPhase;

llvm/lib/Transforms/IPO/Attributor.cpp

ChangeStatus Attributor::run() {
   Phase = AttributorPhase::CLEANUP;
   ChangeStatus CleanupChange = cleanupIR();
 
   return ManifestChange | CleanupChange;
 }
 
 ChangeStatus Attributor::updateAA(AbstractAttribute &AA) {
   TimeTraceScope TimeScope(AA.getName() + "::updateAA");
+  assert(Phase == AttributorPhase::UPDATE &&
+         "We can update AA only in the update stage!");
 
   // Use a new dependence vector for this update.
   DependenceVector DV;
   DependenceStack.push_back(&DV);
 
   auto &AAState = AA.getState();
   ChangeStatus CS = ChangeStatus::UNCHANGED;
   if (!isAssumedDead(AA, nullptr, /* CheckBBLivenessOnly */ true))