[llvm-commits] [PATCH] Memory Dependence Analysis: differenciate "unknown" from "no dependency in current function"

[Guo, Xiaoyi]

Your careful review is much appreciated! See my answers below.


@@ -1364,7 +1366,7 @@
       continue;
     }

-    assert(DepInfo.isDef() && "Expecting def here");
+    // DepInfo.isDef() here

     Instruction *DepInst = DepInfo.getInst();

> What is the point of this change?

There is a isClobber() case in the original code that's missing from the diff. If I add that into the context, the diff would look like this:

@@ -1299,7 +1301,7 @@
     BasicBlock *DepBB = Deps[i].getBB();
     MemDepResult DepInfo = Deps[i].getResult();
 
-    if (DepInfo.isUnknown()) {
+    if (!DepInfo.isDef() && !DepInfo.isClobber()) {
         ...
         Continue;
      }

      If (DepInfo.isClobber()) {
        ...
        Continue;
      }

-    assert(DepInfo.isDef() && "Expecting def here");
+    // DepInfo.isDef() here

     Instruction *DepInst = DepInfo.getInst();

So when it got to the assert(DepInfo.isDef()) point, the assert is not necessary anymore.


> I would say "no known dependency" instead of "no dependency".

Changed.


Attached is the updated patch, with "no dependency" changed to "no known dependency". If it looks okay, I would appreciate it if you could help commit it.

Xiaoyi

-----Original Message-----
From: Eli Friedman [mailto:eli.friedman at gmail.com] 
Sent: Wednesday, October 12, 2011 3:10 PM
To: Guo, Xiaoyi
Cc: llvm-commits at cs.uiuc.edu
Subject: Re: [llvm-commits] [PATCH] Memory Dependence Analysis: differenciate "unknown" from "no dependency in current function"

On Wed, Oct 12, 2011 at 2:20 PM, Guo, Xiaoyi <Xiaoyi.Guo at amd.com> wrote:
> Hi Eli,
>
> Thanks a lot for the feedback. Here's another try according to your suggestion. Please help review it again.

@@ -1364,7 +1366,7 @@
       continue;
     }

-    assert(DepInfo.isDef() && "Expecting def here");
+    // DepInfo.isDef() here

     Instruction *DepInst = DepInfo.getInst();

What is the point of this change?

+      /// Other - This marker indicates that the query has no dependency in
+      /// the specified block.  More detailed state info is encoded in the
+      /// upper part of the pair (i.e. the Instruction*)

I would say "no known dependency" instead of "no dependency".

Otherwise, this patch looks good.

-Eli

> Xiaoyi
>
> -----Original Message-----
> From: Eli Friedman [mailto:eli.friedman at gmail.com]
> Sent: Tuesday, October 11, 2011 5:27 PM
> To: Guo, Xiaoyi
> Cc: llvm-commits at cs.uiuc.edu
> Subject: Re: [llvm-commits] [PATCH] Memory Dependence Analysis: differenciate "unknown" from "no dependency in current function"
>
> On Tue, Oct 11, 2011 at 4:57 PM, Guo, Xiaoyi <Xiaoyi.Guo at amd.com> wrote:
>> If the memory dependence analysis did not find any dependence for a pointer in the current function, it currently returns "Unknown" as the result. However, in the optimization that I'm doing, I need to be able to tell this case apart from a general "unknown" case. i.e., I need to know that the pointer does not have any dependencies in the current function. The patch attached is to achieve this.
>>
>> Please help to review and commit if acceptable.
>
> The interface to MemDepResult::getNonFuncLocal makes no sense: as far as I can tell, the Instruction* which is passed in is useless except for the fact that it isn't null.
>
> MemDepResult's internal representation is starting to get a bit crazy... with your changes, there are 6 states: Invalid, Def, Clobber, NonLocal, NonLocalFunc, and Unknown.  Def and Clobber have an associated instruction, the other states do not.  Granted, I sort of started the craziness by the way Unknown is represented, but this is starting to become a complete mess; maybe we can restore a bit of sanity by splitting things up so that the bottom part of the pair is "Def", "Clobber", or "Other", and encode the other states into upper part of "Other", or something like that?
>
> -Eli
>
>

-------------- next part --------------
A non-text attachment was scrubbed...
Name: MemoryDependenceAnalysis.diff
Type: application/octet-stream
Size: 14568 bytes
Desc: MemoryDependenceAnalysis.diff
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20111012/7760dd99/attachment.obj>