[PATCH] D127202: [InlineFunction] Call to getUnderlyingObjects inside AddAliasScopeMetadata shall set MaxLookup = 0

[Ting Wang via Phabricator via llvm-commits]

tingwang created this revision.
tingwang added reviewers: hfinkel, anemet, Eugene.Zelenko.
tingwang added a project: LLVM.
Herald added subscribers: jeroen.dobbelaere, hiraditya.
Herald added a project: All.
tingwang requested review of this revision.
Herald added a subscriber: llvm-commits.

>From historical perspective:

// initial commit on Fri Jul 25 15:50:08 2014
ff0bcb60c9839c81608b5279909ec330a392c9fd
+        GetUnderlyingObjects(const_cast<Value*>(PtrArgs[i]),
+                             Objects, DL, /* MaxLookup = */ 0);

// API changed on Thu Apr 23 20:09:20 2015
e2b885c4bc58b56d2fb3d0e6b3dd8e932742cf73

  void GetUnderlyingObjects(Value *V, SmallVectorImpl<Value *> &Objects,

- const DataLayout &DL, unsigned MaxLookup = 6);

+                            const DataLayout &DL, LoopInfo *LI = nullptr,
+                            unsigned MaxLookup = 6);

// Comments and tidy update
083ca9bb3233fb4df575bfb8c4198e3f9d6e3acd

  GetUnderlyingObjects(const_cast<Value*>(PtrArgs[i]),

- Objects, DL, /* MaxLookup = */ 0);

+                             Objects, DL, /* LI = */ nullptr);

It seems initial intention was to call GetUnderlyingObjects() (now renamed to getUnderlyingObjects()) with unlimited loop attempt. However second commit changed API arguments, and no update happened to the invoke site in AddAliasScopeMetadata, at that point the logic had changed, and the default MaxLookup = 6 applied on the call. The third commit completely wiped out the information related with initial intention.

Recently, we hit upon one test case (attached test.ll) blocked by this MaxLookup = 6 in AddAliasScopeMetadata: the default value of MaxLookup is not enough to identify the underlying object, and the alias info added by AddAliasScopeMetadata() is wrong, resulting in wrong schedule decisions in machine-scheduler. Information regarding the test case can be found in below link:
https://reviews.llvm.org/D86669#3559559

The current situation in my case is: manually I can confirm the troubling pointer points to argument with noalias attribute, however getUnderlyingObjects() failed to confirm that fact due to MaxLookup limit.

Digging through history, I feel like restoring the initial setting should be a solution to both my issue and the logical flaw created in history.


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D127202

Files:
  llvm/lib/Transforms/Utils/InlineFunction.cpp


Index: llvm/lib/Transforms/Utils/InlineFunction.cpp
===================================================================
--- llvm/lib/Transforms/Utils/InlineFunction.cpp
+++ llvm/lib/Transforms/Utils/InlineFunction.cpp
@@ -1071,7 +1071,8 @@
       SmallSetVector<const Argument *, 4> NAPtrArgs;
       for (const Value *V : PtrArgs) {
         SmallVector<const Value *, 4> Objects;
-        getUnderlyingObjects(V, Objects, /* LI = */ nullptr);
+        getUnderlyingObjects(V, Objects, /* LI = */ nullptr,
+                             /* MaxLookup = */ 0);
 
         for (const Value *O : Objects)
           ObjSet.insert(O);


-------------- next part --------------
A non-text attachment was scrubbed...
Name: D127202.434758.patch
Type: text/x-patch
Size: 630 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20220607/5b380c7e/attachment.bin>