[PATCH] D143228: [Attributor] Add convergent abstract attribute

[Johannes Doerfert via Phabricator via llvm-commits]

ishaangandhi created this revision.
Herald added subscribers: ormris, okura, jdoerfert, kuter, hiraditya.
Herald added a project: All.
ishaangandhi changed the visibility from "Public (No Login Required)" to "No One".
ishaangandhi removed subscribers: hiraditya, kuter, jdoerfert, okura, ormris.
ishaangandhi updated this revision to Diff 494634.
ishaangandhi added a comment.
ishaangandhi updated this revision to Diff 494846.
ishaangandhi retitled this revision from "Something that compiles" to "[Attributor] Add convergent abstract attribute".
ishaangandhi updated this revision to Diff 494849.
ishaangandhi changed the visibility from "No One" to "All Users".
Herald added a reviewer: homerdin.
ishaangandhi updated this revision to Diff 495980.
ishaangandhi marked 7 inline comments as done.
ishaangandhi marked 2 inline comments as not done.
ishaangandhi updated this revision to Diff 499294.
ishaangandhi marked 4 inline comments as done.
ishaangandhi updated this revision to Diff 499298.
ishaangandhi marked 2 inline comments as done.
ishaangandhi added a comment.
ishaangandhi updated this revision to Diff 499305.
ishaangandhi updated this revision to Diff 499652.
ishaangandhi marked 4 inline comments as done.
ishaangandhi updated this revision to Diff 499661.
ishaangandhi updated this revision to Diff 499705.
ishaangandhi marked 3 inline comments as done.
ishaangandhi updated this revision to Diff 500259.
ishaangandhi updated this revision to Diff 500450.
ishaangandhi updated this revision to Diff 500458.
Herald added a subscriber: steven_wu.
ishaangandhi updated this revision to Diff 500954.
ishaangandhi marked 5 inline comments as done.
ishaangandhi updated this revision to Diff 501169.
ishaangandhi updated this revision to Diff 501213.
jdoerfert added a reviewer: jdoerfert.
jdoerfert published this revision for review.
Herald added a reviewer: sstefan1.
Herald added subscribers: llvm-commits, sstefan1.
Herald added a project: LLVM.

Add CheckForConvergent


ishaangandhi added a comment.

Use norecurse style, add tests


ishaangandhi added a comment.

[Attributor] Add convergent abstract attribute


jdoerfert added a comment.

We need more tests, like an scc in which all functions are convergent initially but none after because they don't call a convergent declaration. Then one were they do. Some more initial comments inlined.


ishaangandhi added a comment.

- Add attributor: Code review #1
  - change "nonconvergent" -> "non-convergent"
  - Give up in a convergent declaration
  - Remove Call site AA
  - Remove Edge reachability code
  - Add convergent declarations to test


ishaangandhi added a comment.

- Add attributor: Code review #2


ishaangandhi added a comment.

(Still need to add this in the `openmp-opt` pass and add more tests as requested.)


ishaangandhi added a comment.

- Add convergent attribute: Code review #2

seed AA in OpenMPOpt


ishaangandhi added a comment.

- Add convergent attribute: Code review #2


ishaangandhi added a comment.



  Add Convergent AA: Code review #4
  
  * Fix switched isAssumedNotConvergent bug
  * Make ConvergentAA DepClassTy::REQUIRED
  * Add missing return for indicatePessimisticFixpoint()


ishaangandhi added a comment.

- Add convergent, remove opaque-pointers from test


ishaangandhi added a comment.

- Use StateWrapper instead of IRAttribute
- Run git-clang-format main


ishaangandhi added a comment.

- Use checkForAllCallLikeInstructions instead of checkForAllCallSites


ishaangandhi added a comment.

- Remove unneeded debug logging


ishaangandhi added a comment.

- Format AttributorAttributes.cpp
- Update test checks for OpenMP bitcode


jdoerfert added a comment.

Almost there. Some more tests would be good and I left some minor comments.


ishaangandhi added a comment.

- Rename to AANonConvergent, add tests


ishaangandhi added a comment.

- Fix typo in function name in test
- Update test checks to remove convergent attr


ishaangandhi added a comment.

- Run git-clang-format



================
Comment at: llvm/include/llvm/Transforms/IPO/Attributor.h:5113
+/// attribute.
+struct AAConvergent : public StateWrapper<BooleanState, AbstractAttribute> {
+  using Base = StateWrapper<BooleanState, AbstractAttribute>;
----------------
Can we rename it in AANonConvergent, I think that makes it clearer what is deduced here.


================
Comment at: llvm/include/llvm/Transforms/IPO/Attributor.h:5128
+  /// See AbstractAttribute::getName()
+  const std::string getName() const override { return "AAConvergent"; }
+
----------------
^ also here


================
Comment at: llvm/include/llvm/Transforms/IPO/Attributor.h:5114
+    : public IRAttribute<Attribute::Convergent,
+                         StateWrapper<BooleanState, AbstractAttribute>> {
+
----------------
I think this is the issue. All other attributes are (correctly) designed such that having the attribute is "good". Convergent is (unfortunately) different.
Don't make it an IRAttribute, just use the StateWrapper.


================
Comment at: llvm/include/llvm/Transforms/IPO/Attributor.h:5120
+  static AAConvergent &createForPosition(const IRPosition &IRP,
+                                                  Attributor &A);
+
----------------
clang format the patch. `git-clang-format HEAD~` is what I usually use.


================
Comment at: llvm/include/llvm/Transforms/IPO/Attributor.h:5126
+  /// Return true if "non-convergent" is known.
+  bool isKnownNotConvergent() const { return getAssumed(); }
+
----------------
You have known and assumed switched. 


================
Comment at: llvm/include/llvm/Transforms/IPO/Attributor.h:5126
+  /// Return true if "non-convergent" is known.
+  bool isKnownNotConvergent() const { return !getAssumed(); }
+
----------------
the optimistic state (so, getAssumed() == true), should mean "non-convergent" and the pessimistic state should be convergent. While you can flip it, as you did, it is confusing as we otherwise make say "getAssume() == true" ==> good.


================
Comment at: llvm/include/llvm/Transforms/IPO/Attributor.h:5126
+  /// Return true if "convergent" is known.
+  bool isKnownConvergent() const { return getKnown(); }
+
----------------
We probably want to rename these to not convergent. Pessimistic is convergent, optimistic is not.


================
Comment at: llvm/lib/Transforms/IPO/Attributor.cpp:3164
 
+  // Every function can be "convergent".
+  getOrCreateAAFor<AAConvergent>(FPos);
----------------



================
Comment at: llvm/lib/Transforms/IPO/Attributor.cpp:3164
 
+  // Every function can be "convergent".
+  getOrCreateAAFor<AAConvergent>(FPos);
----------------
Add this in the openmp-opt pass as well for device code.


================
Comment at: llvm/lib/Transforms/IPO/Attributor.cpp:3164
 
+  // Every function can be "convergent".
+  getOrCreateAAFor<AAConvergent>(FPos);
----------------
jdoerfert wrote:
> Add this in the openmp-opt pass as well for device code.
Would that involve editing `llvm/lib/Transforms/IPO/OpenMPOpt.cpp`?


================
Comment at: llvm/lib/Transforms/IPO/Attributor.cpp:3164
 
+  // Every function can be "convergent".
+  getOrCreateAAFor<AAConvergent>(FPos);
----------------
ishaangandhi wrote:
> jdoerfert wrote:
> > Add this in the openmp-opt pass as well for device code.
> Would that involve editing `llvm/lib/Transforms/IPO/OpenMPOpt.cpp`?
Yes. We create/seed AAs there too


================
Comment at: llvm/lib/Transforms/IPO/AttributorAttributes.cpp:2941-2943
+      if (!Callee || Callee->isDeclaration()) {
+        return false;
+      }
----------------
If the callee is unknown, we can give up. However, if it's a declaration we can check for the attribute:
```
if (!Callee)
  return false;
if (Callee->isDeclaration())
  return !Callee->hasFnAttr(Attribute::Convergent);
```  


================
Comment at: llvm/lib/Transforms/IPO/AttributorAttributes.cpp:2955
+      // non-convergent. If one of the calls we have not visited will become
+      // live, another update is triggered.
+      return indicatePessimisticFixpoint();
----------------
The first 2 sentences of this comment don't make sense to me. Maybe remove the comment or try to update it after all the code changes.


================
Comment at: llvm/lib/Transforms/IPO/AttributorAttributes.cpp:2961
+  void trackStatistics() const override { STATS_DECLTRACK_FN_ATTR(convergent) }
+};
+} // namespace
----------------
Add a manifest function (overload) that removes convergent from the associated function. It is only called if the attribute actually is assumed to be non-convergent at the end of the fix point iteration.


================
Comment at: llvm/lib/Transforms/IPO/AttributorAttributes.cpp:2945
+      const auto &ConvergentAA = A.getAAFor<AAConvergent>(
+          *this, IRPosition::function(*Callee), DepClassTy::NONE);
+      return ConvergentAA.isAssumedNotConvergent();
----------------



================
Comment at: llvm/lib/Transforms/IPO/AttributorAttributes.cpp:2956
+      // live, another update is triggered.
+      indicatePessimisticFixpoint();
+    }
----------------



================
Comment at: llvm/lib/Transforms/IPO/AttributorAttributes.cpp:2958-2959
+    }
+    else if (UsedAssumedInformation)
+      indicateOptimisticFixpoint();
+    return ChangeStatus::UNCHANGED;
----------------
I don't get why this should indicate a fix point. 


================
Comment at: llvm/lib/Transforms/IPO/AttributorAttributes.cpp:2943
+          DepClassTy::NONE);
+      return ConvergentAA.isKnownNotConvergent();
+    };
----------------
You want to use the assumed value, not the known one. Also, consider to first collect all callees in a set to avoid looking up the same AA multiple times. Finally, your code actually looks up the AA for this function because ACS.getInstruction() is the call site instruction, and getFunction will give the function this AA is associated with. What you want is ACS.getCallee (or similar) [which can be null/unknown].


================
Comment at: llvm/lib/Transforms/IPO/AttributorAttributes.cpp:2955
+        indicateOptimisticFixpoint();
+    }
+    return ChangeStatus::UNCHANGED;
----------------
This update function will always stay at assume non-convergent. The patter you want is:
```
if (!A.checkForAll...)
  return indicatePessimisticFixpoint();
```
So if we failed to visit all call sites or one call sites was convergent, we will give up on the non-convergent for this function.


================
Comment at: llvm/lib/Transforms/IPO/AttributorAttributes.cpp:2926
+  /// See AbstractAttribute::getAsStr()
+  const std::string getAsStr() const override {
+    return getAssumed() ? "nonconvergent" : "may-be-convergent";
----------------
Non-


================
Comment at: llvm/lib/Transforms/IPO/AttributorAttributes.cpp:2943
+          DepClassTy::NONE);
+      return !ConvergentAA.isKnownConvergent();
+    };
----------------
I think we can even simply this part. We give up if it's a convergent declaration.


================
Comment at: llvm/lib/Transforms/IPO/AttributorAttributes.cpp:2956
+        indicatePessimisticFixpoint();
+      return ChangeStatus::UNCHANGED;
+    }
----------------
If the callback failed we need to give up. I don't understand how we could continue. Check the "for all" documentation.


================
Comment at: llvm/lib/Transforms/IPO/AttributorAttributes.cpp:2965
+    if (EdgeReachability.canReach(A, *getAnchorScope()))
+      return indicateOptimisticFixpoint();
+    return ChangeStatus::UNCHANGED;
----------------
We don't need this part.


================
Comment at: llvm/lib/Transforms/IPO/AttributorAttributes.cpp:2999
+  void trackStatistics() const override { STATS_DECLTRACK_CS_ATTR(convergent); }
+};
+} // namespace
----------------
If we directly check the call base we can avoid the call site AA completely


================
Comment at: llvm/lib/Transforms/IPO/AttributorAttributes.cpp:2999
+  void trackStatistics() const override { STATS_DECLTRACK_CS_ATTR(convergent); }
+};
+} // namespace
----------------
jdoerfert wrote:
> If we directly check the call base we can avoid the call site AA completely
Could you clarify how I would directly check the call base? I started by removing the call site AA.


================
Comment at: llvm/test/Transforms/Attributor/convergent.ll:20
+;
+; CGSCC: Function Attrs: convergent nofree nosync nounwind willreturn memory(none)
+; CGSCC-LABEL: define {{[^@]+}}@calls_defined
----------------
Calling this out. In CGSCC, the `convergent` attribute remains. Its not in the `TUNIT` check though.


================
Comment at: llvm/test/Transforms/Attributor/convergent.ll:12
+  ret i32 1
+}
+
----------------
Add an initially convergent function calling leaf here too.
Also add a second test like below but only the caller, not the declared callee, is convergent. Thus,
```
declare void @harmless()
define void @caller() convergent {
  call void @harmless()
}
```


================
Comment at: llvm/test/Transforms/Attributor/convergent.ll:16
+; CHECK-NEXT: declare i32 @k()
+declare i32 @k() readnone
+
----------------
Why isn't this declaration getting a "convergent" attribute?


================
Comment at: llvm/test/Transforms/Attributor/convergent.ll:16
+; CHECK-NEXT: declare i32 @k()
+declare i32 @k() readnone
+
----------------
ishaangandhi wrote:
> Why isn't this declaration getting a "convergent" attribute?
Add it manually. The frontend ads it based on the language and target semantic. No pass will add them. 


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D143228

Files:
  llvm/include/llvm/Transforms/IPO/Attributor.h
  llvm/lib/Transforms/IPO/Attributor.cpp
  llvm/lib/Transforms/IPO/AttributorAttributes.cpp
  llvm/lib/Transforms/IPO/OpenMPOpt.cpp
  llvm/test/Transforms/Attributor/convergent.ll
  llvm/test/Transforms/OpenMP/always_inline_device.ll
  llvm/test/Transforms/OpenMP/custom_state_machines.ll
  llvm/test/Transforms/OpenMP/custom_state_machines_pre_lto.ll
  llvm/test/Transforms/OpenMP/spmdization.ll
  llvm/test/Transforms/OpenMP/spmdization_guarding_two_reaching_kernels.ll
  llvm/test/Transforms/OpenMP/spmdization_no_guarding_two_reaching_kernels.ll

-------------- next part --------------
A non-text attachment was scrubbed...
Name: D143228.501213.patch
Type: text/x-patch
Size: 183626 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20230303/7eb169c4/attachment-0001.bin>