[PATCH] D74691: [Attributor] add some pattern to containsCycle

Tue Feb 18 08:44:15 PST 2020

baziotis added a comment.

In D74691#1880781 <https://reviews.llvm.org/D74691#1880781>, @omarahmed wrote:

>   Failing Tests (74):
>       LLVM :: Transforms/Attributor/ArgumentPromotion/2008-02-01-ReturnAttrs.ll
>       LLVM :: Transforms/Attributor/ArgumentPromotion/2008-07-02-array-indexing.ll
>       LLVM :: Transforms/Attributor/ArgumentPromotion/2008-09-07-CGUpdate.ll
>       LLVM :: Transforms/Attributor/ArgumentPromotion/2008-09-08-CGUpdateSelfEdge.ll
>       LLVM :: Transforms/Attributor/ArgumentPromotion/X86/attributes.ll
>       LLVM :: Transforms/Attributor/ArgumentPromotion/X86/min-legal-vector-width.ll
>       LLVM :: Transforms/Attributor/ArgumentPromotion/aggregate-promote.ll
>       LLVM :: Transforms/Attributor/ArgumentPromotion/alignment.ll
>       LLVM :: Transforms/Attributor/ArgumentPromotion/attrs.ll
>       LLVM :: Transforms/Attributor/ArgumentPromotion/basictest.ll
>       LLVM :: Transforms/Attributor/ArgumentPromotion/byval-2.ll
>       LLVM :: Transforms/Attributor/ArgumentPromotion/byval.ll
>       LLVM :: Transforms/Attributor/ArgumentPromotion/chained.ll
>       LLVM :: Transforms/Attributor/ArgumentPromotion/control-flow.ll
>       LLVM :: Transforms/Attributor/ArgumentPromotion/control-flow2.ll
>       LLVM :: Transforms/Attributor/ArgumentPromotion/crash.ll
>       LLVM :: Transforms/Attributor/ArgumentPromotion/dbg.ll
>       LLVM :: Transforms/Attributor/ArgumentPromotion/fp80.ll
>       LLVM :: Transforms/Attributor/ArgumentPromotion/inalloca.ll
>       LLVM :: Transforms/Attributor/ArgumentPromotion/live_called_from_dead.ll
>       LLVM :: Transforms/Attributor/ArgumentPromotion/live_called_from_dead_2.ll
>       LLVM :: Transforms/Attributor/ArgumentPromotion/musttail.ll
>       LLVM :: Transforms/Attributor/ArgumentPromotion/naked_functions.ll
>       LLVM :: Transforms/Attributor/ArgumentPromotion/nonzero-address-spaces.ll
>       LLVM :: Transforms/Attributor/ArgumentPromotion/pr27568.ll
>       LLVM :: Transforms/Attributor/ArgumentPromotion/pr3085.ll
>       LLVM :: Transforms/Attributor/ArgumentPromotion/pr32917.ll
>       LLVM :: Transforms/Attributor/ArgumentPromotion/pr33641_remove_arg_dbgvalue.ll
>       LLVM :: Transforms/Attributor/ArgumentPromotion/profile.ll
>       LLVM :: Transforms/Attributor/ArgumentPromotion/reserve-tbaa.ll
>       LLVM :: Transforms/Attributor/ArgumentPromotion/sret.ll
>       LLVM :: Transforms/Attributor/ArgumentPromotion/tail.ll
>       LLVM :: Transforms/Attributor/ArgumentPromotion/variadic.ll
>       LLVM :: Transforms/Attributor/IPConstantProp/2009-09-24-byval-ptr.ll
>       LLVM :: Transforms/Attributor/IPConstantProp/PR16052.ll
>       LLVM :: Transforms/Attributor/IPConstantProp/PR26044.ll
>       LLVM :: Transforms/Attributor/IPConstantProp/PR43857.ll
>       LLVM :: Transforms/Attributor/IPConstantProp/arg-count-mismatch.ll
>       LLVM :: Transforms/Attributor/IPConstantProp/arg-type-mismatch.ll
>       LLVM :: Transforms/Attributor/IPConstantProp/comdat-ipo.ll
>       LLVM :: Transforms/Attributor/IPConstantProp/dangling-block-address.ll
>       LLVM :: Transforms/Attributor/IPConstantProp/fp-bc-icmp-const-fold.ll
>       LLVM :: Transforms/Attributor/IPConstantProp/global.ll
>       LLVM :: Transforms/Attributor/IPConstantProp/multiple_callbacks.ll
>       LLVM :: Transforms/Attributor/IPConstantProp/musttail-call.ll
>       LLVM :: Transforms/Attributor/IPConstantProp/naked-return.ll
>       LLVM :: Transforms/Attributor/IPConstantProp/openmp_parallel_for.ll
>       LLVM :: Transforms/Attributor/IPConstantProp/pthreads.ll
>       LLVM :: Transforms/Attributor/IPConstantProp/recursion.ll
>       LLVM :: Transforms/Attributor/IPConstantProp/remove-call-inst.ll
>       LLVM :: Transforms/Attributor/IPConstantProp/return-argument.ll
>       LLVM :: Transforms/Attributor/IPConstantProp/return-constant.ll
>       LLVM :: Transforms/Attributor/IPConstantProp/return-constants.ll
>       LLVM :: Transforms/Attributor/IPConstantProp/solve-after-each-resolving-undefs-for-function.ll
>       LLVM :: Transforms/Attributor/IPConstantProp/thread_local_acs.ll
>       LLVM :: Transforms/Attributor/align.ll
>       LLVM :: Transforms/Attributor/callbacks.ll
>       LLVM :: Transforms/Attributor/heap_to_stack.ll
>       LLVM :: Transforms/Attributor/internal-noalias.ll
>       LLVM :: Transforms/Attributor/liveness.ll
>       LLVM :: Transforms/Attributor/liveness_chains.ll
>       LLVM :: Transforms/Attributor/lvi-after-jumpthreading.ll
>       LLVM :: Transforms/Attributor/lvi-for-ashr.ll
>       LLVM :: Transforms/Attributor/memory_locations.ll
>       LLVM :: Transforms/Attributor/misc.ll
>       LLVM :: Transforms/Attributor/noalias.ll
>       LLVM :: Transforms/Attributor/nocapture-1.ll
>       LLVM :: Transforms/Attributor/nonnull.ll
>       LLVM :: Transforms/Attributor/norecurse.ll
>       LLVM :: Transforms/Attributor/range.ll
>       LLVM :: Transforms/Attributor/read_write_returned_arguments_scc.ll
>       LLVM :: Transforms/Attributor/readattrs.ll
>       LLVM :: Transforms/Attributor/reduced/register_benchmark_test.ll
>       LLVM :: Transforms/Attributor/willreturn.ll
>  
>     Expected Passes    : 17
>     Unexpected Failures: 74
>  
>
>
> the code I have updated fails in nearly all the tests , I think all the tests fails because the function always returns true not false so what could possibly result in that ?

That seems weird because now you should return true less or equal times than before. Consider that with the previous approach, if there was an SCC //at all//, it would return true.
So, it would make sense that test fail because more falses are returned, not the inverse. But in any case, you may want to put some printfs and test that (maybe fire an assert when the new version and the old version return a different result and see why that happens).

Second, be sure to check Johannes' advice about formatting with clang format.

================
Comment at: llvm/lib/Transforms/IPO/Attributor.cpp:2376-2380
+        A.getInfoCache().getAnalysisResultForFunction<ScalarEvolutionAnalysis>(
+            F);
+    LoopInfo *LI =
+        A.getInfoCache().getAnalysisResultForFunction<LoopAnalysis>(F);
+    if (!LI || !SE) continue;
----------------
I believe you can get all those (LI, SE and their tests for null) out (i.e. before) of the loop, I don't think they can change.

================
Comment at: llvm/lib/Transforms/IPO/Attributor.cpp:2383
+    Loop *L = LI->getLoopFor(*BBI);
+    L = L->getParentLoop();
+    if (L) {
----------------
You don't want to do this here. You first want to test `L` before you test with its parent because `L` might be equal (in blocks) with your SCC.
I think, following the cases I listed can help.
First of all, try to get `L`. If you don't have a loop (so, `L` is null) then this
SCC is not a loop so return true immediately.
Otherwise, test their sizes (SCC and L, in blocks). And the below cases follow:
- If they're equal:
  - If there's a max trip count, continue with the next SCC.
  - Otherwise, return true.
- Otherwise, if the SCC is smaller,  return true
- Otherwise (i.e. the SCC is bigger), get the parent loop and do the same
  steps for the parent loop. Note that this last case effectively implies a loop.

Finally, if the SCCs end, return false (i.e. in the end of the function).

Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D74691/new/

https://reviews.llvm.org/D74691