llvm dev - Sep 2012 - [LLVMdev] teaching FileCheck to handle variations in order

Hello all,

For the hexagon target, we have a couple of tests that are failing due 
to variations in the order of checked text. In these cases the ordering 
is not directly relevant to the functionality being tested.

For example:

    ; CHECK: memw(##a)
    ; CHECK: memw(##b)

    %0 = load i32* @a, align 4
    %1 = load i32* @b, align 4

requires that the compiler emit the memory operations for 'a' and
'b' in
that order, even though the intent of the test might simply be to ensure 
that each 'load' results in a memory read.

I'd like to teach FileCheck to allow tests to be more tolerant of these 
incidental variations.

The attached patch implements one possible solution. It introduces a 
position stack and a couple of directives:

  * 'CHECK-PUSH:' pushes the current match position onto the stack.
  * 'CHECK-POP:' pops the top value off of the stack and uses it to set
    the current match position.

The above test can now be re-written as:

    ; CHECK-PUSH:
    ; CHECK: memw(##a)
    ; CHECK-POP:
    ; CHECK: memw(##b)

    %0 = load i32* @a, align 4
    %1 = load i32* @b, align 4

which handles either ordering of memory reads for 'a' and 'b'.

Thoughts?

Thanks,
Matthew Curtis

-- 
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum, hosted by The
Linux Foundation

-------------- next part --------------
An HTML attachment was scrubbed...
URL:
<http://lists.llvm.org/pipermail/llvm-dev/attachments/20120907/dc2bf4af/attachment.html>
-------------- next part --------------
diff --git a/utils/FileCheck/FileCheck.cpp b/utils/FileCheck/FileCheck.cpp
index 33f04ce..9f6a64a 100644
--- a/utils/FileCheck/FileCheck.cpp
+++ b/utils/FileCheck/FileCheck.cpp
@@ -50,11 +50,23 @@ NoCanonicalizeWhiteSpace("strict-whitespace",
 //===----------------------------------------------------------------------===//
 
 class Pattern {
+public:
+  enum MatchType {
+    MatchStr,
+    MatchCurrent,
+    MatchEndOfFile
+  };
+
+private:
   SMLoc PatternLoc;
 
-  /// MatchEOF - When set, this pattern only matches the end of file. This is
-  /// used for trailing CHECK-NOTs.
-  bool MatchEOF;
+  /// MatchType - When set to ...
+  ///   MatchStr, this pattern matches according to FixedStr or RegExStr.
+  ///   MatchCurrent, this pattern matches (the empty string at) the current
+  ///     position. This is used for CHECK-PUSHes without preceding CHECKs.
+  ///   MatchEndOfFile, this pattern only matches the end of file. This is used
+  ///     for trailing CHECK-NOTs.
+  enum MatchType Type;
 
   /// FixedStr - If non-empty, this pattern is a fixed string match with the
   /// specified fixed string.
@@ -77,7 +89,7 @@ class Pattern {
 
 public:
 
-  Pattern(bool matchEOF = false) : MatchEOF(matchEOF) { }
+  Pattern(enum MatchType t = MatchStr) : Type(t) { }
 
   bool ParsePattern(StringRef PatternStr, SourceMgr &SM);
 
@@ -285,9 +297,18 @@ bool Pattern::AddRegExToRegEx(StringRef RegexStr, unsigned
&CurParen,
 size_t Pattern::Match(StringRef Buffer, size_t &MatchLen,
                       StringMap<StringRef> &VariableTable) const {
   // If this is the EOF pattern, match it immediately.
-  if (MatchEOF) {
+  switch (Type) {
+  case MatchStr:
+    break;
+  case MatchCurrent:
+    MatchLen = 0;
+    return 0;
+  case MatchEndOfFile:
     MatchLen = 0;
     return Buffer.size();
+  default:
+    assert("Unknown match type");
+    break;
   }
 
   // If this is a fixed string pattern, just match it now.
@@ -447,6 +468,9 @@ struct CheckString {
   /// IsCheckNext - This is true if this is a CHECK-NEXT: directive (as opposed
   /// to a CHECK: directive.
   bool IsCheckNext;
+  int PushPos;
+  int PopPos;
+  SMLoc PopLoc;
 
   /// NotStrings - These are all of the strings that are disallowed from
   /// occurring between this match string and the previous one (or start of
@@ -454,7 +478,7 @@ struct CheckString {
   std::vector<std::pair<SMLoc, Pattern> > NotStrings;
 
   CheckString(const Pattern &P, SMLoc L, bool isCheckNext)
-    : Pat(P), Loc(L), IsCheckNext(isCheckNext) {}
+    : Pat(P), Loc(L), IsCheckNext(isCheckNext), PushPos(0), PopPos(0) {}
 };
 
 /// CanonicalizeInputFile - Remove duplicate horizontal space from the
specified
@@ -517,6 +541,8 @@ static bool ReadCheckFile(SourceMgr &SM,
   StringRef Buffer = F->getBuffer();
 
   std::vector<std::pair<SMLoc, Pattern> > NotMatches;
+  int NextCheckPopPos= 0;
+  SMLoc NextCheckPopLoc;
 
   while (1) {
     // See if Prefix occurs in the memory buffer.
@@ -531,6 +557,7 @@ static bool ReadCheckFile(SourceMgr &SM,
     // When we find a check prefix, keep track of whether we find CHECK: or
     // CHECK-NEXT:
     bool IsCheckNext = false, IsCheckNot = false;
+    bool IsCheckPush = false, IsCheckPop = false;
 
     // Verify that the : is present after the prefix.
     if (Buffer[CheckPrefix.size()] == ':') {
@@ -543,6 +570,14 @@ static bool ReadCheckFile(SourceMgr &SM,
                memcmp(Buffer.data()+CheckPrefix.size(), "-NOT:", 5)
== 0) {
       Buffer = Buffer.substr(CheckPrefix.size()+6);
       IsCheckNot = true;
+    } else if (Buffer.size() > CheckPrefix.size()+6 &&
+               memcmp(Buffer.data()+CheckPrefix.size(), "-PUSH:", 6)
== 0) {
+      Buffer = Buffer.substr(CheckPrefix.size()+7);
+      IsCheckPush = true;
+    } else if (Buffer.size() > CheckPrefix.size()+5 &&
+               memcmp(Buffer.data()+CheckPrefix.size(), "-POP:", 5)
== 0) {
+      Buffer = Buffer.substr(CheckPrefix.size()+6);
+      IsCheckPop = true;
     } else {
       Buffer = Buffer.substr(1);
       continue;
@@ -555,6 +590,22 @@ static bool ReadCheckFile(SourceMgr &SM,
     // Scan ahead to the end of line.
     size_t EOL = Buffer.find_first_of("\n\r");
 
+    if (IsCheckPush) {
+      if (CheckStrings.empty()) {
+        CheckStrings.push_back(CheckString(Pattern(Pattern::MatchCurrent),
+                                          
SMLoc::getFromPointer(Buffer.data()),
+                                           false));
+      }
+      CheckStrings.back().PushPos++;
+      continue;
+    }
+
+    if (IsCheckPop) {
+      NextCheckPopLoc = SMLoc::getFromPointer(CheckPrefixStart);
+      NextCheckPopPos++;
+      continue;
+    }
+
     // Remember the location of the start of the pattern, for diagnostics.
     SMLoc PatternLoc = SMLoc::getFromPointer(Buffer.data());
 
@@ -587,14 +638,22 @@ static bool ReadCheckFile(SourceMgr &SM,
     CheckStrings.push_back(CheckString(P,
                                        PatternLoc,
                                        IsCheckNext));
+    CheckStrings.back().PopPos= NextCheckPopPos;
+    CheckStrings.back().PopLoc= NextCheckPopLoc;
+    NextCheckPopPos= 0;
+    NextCheckPopLoc= SMLoc();
     std::swap(NotMatches, CheckStrings.back().NotStrings);
   }
 
   // Add an EOF pattern for any trailing CHECK-NOTs.
   if (!NotMatches.empty()) {
-    CheckStrings.push_back(CheckString(Pattern(true),
+    CheckStrings.push_back(CheckString(Pattern(Pattern::MatchEndOfFile),
                                        SMLoc::getFromPointer(Buffer.data()),
                                        false));
+    CheckStrings.back().PopPos= NextCheckPopPos;
+    CheckStrings.back().PopLoc= NextCheckPopLoc;
+    NextCheckPopPos= 0;
+    NextCheckPopLoc= SMLoc();
     std::swap(NotMatches, CheckStrings.back().NotStrings);
   }
 
@@ -686,10 +745,22 @@ int main(int argc, char **argv) {
   StringRef Buffer = F->getBuffer();
 
   const char *LastMatch = Buffer.data();
+  std::vector<StringRef> BufferStack;
 
   for (unsigned StrNo = 0, e = CheckStrings.size(); StrNo != e; ++StrNo) {
     const CheckString &CheckStr = CheckStrings[StrNo];
 
+    for (int i = CheckStr.PopPos; i; --i) {
+      if (BufferStack.empty()) {
+        SM.PrintMessage(CheckStr.PopLoc, SourceMgr::DK_Error,
+                        "attempting to
'"+CheckPrefix+"-POP:' without previous '"+
+                        CheckPrefix+ "-PUSH:'");
+        return 1;
+      }
+      Buffer= BufferStack.back();
+      BufferStack.pop_back();
+    }
+
     StringRef SearchFrom = Buffer;
 
     // Find StrNo in the file.
@@ -756,6 +827,9 @@ int main(int argc, char **argv) {
     // the position after the match as the end of the last match.
     Buffer = Buffer.substr(MatchLen);
     LastMatch = Buffer.data();
+    for (int i= CheckStr.PushPos; i; --i) {
+      BufferStack.push_back(Buffer);
+    }
   }
 
   return 0;

On 9/7/2012 7:20 AM, Matthew Curtis wrote:
>
> The attached patch implements one possible solution. It introduces a
> position stack and a couple of directives:
>
>   * 'CHECK-PUSH:' pushes the current match position onto the stack.
>   * 'CHECK-POP:' pops the top value off of the stack and uses it to
set
>     the current match position.
>
> The above test can now be re-written as:
>
>     ; CHECK-PUSH:
>     ; CHECK: memw(##a)
>     ; CHECK-POP:
>     ; CHECK: memw(##b)
>
>     %0 = load i32* @a, align 4
>     %1 = load i32* @b, align 4
>
> which handles either ordering of memory reads for 'a' and
'b'.
>
> Thoughts?
I'm not sure if I got the details of how the tests work, but wouldn't 
this allow false positives?

Take this, for example:

; CHECK-PUSH:
; CHECK: memw(##a)
; CHECK-POP:
; CHECK: memw(##b)

%0 = load i32* @a, align 4
%1 = load i32* @b, align 4

; CHECK: memw(##a)

%2 = load i32* @a, align 4


My understanding is that this is supposed to detect:
- 2 loads in any order, one from @a, the other from @b, followed by
- 1 load from @a,
that is, a total of 3 loads.

At the same time I believe that it would positively match this code:
   ... = memw(##b)
   ... = memw(##a)
i.e. only two loads.  The second load would match two different CHECKs.

-K

-- 
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum, 
hosted by The Linux Foundation

On 9/7/2012 12:12 PM, Krzysztof Parzyszek wrote:
> On 9/7/2012 7:20 AM, Matthew Curtis wrote:
>>
>> The attached patch implements one possible solution. It introduces a
>> position stack and a couple of directives:
>>
>>   * 'CHECK-PUSH:' pushes the current match position onto the
stack.
>>   * 'CHECK-POP:' pops the top value off of the stack and uses
it to set
>>     the current match position.
>>
>> The above test can now be re-written as:
>>
>>     ; CHECK-PUSH:
>>     ; CHECK: memw(##a)
>>     ; CHECK-POP:
>>     ; CHECK: memw(##b)
>>
>>     %0 = load i32* @a, align 4
>>     %1 = load i32* @b, align 4
>>
>> which handles either ordering of memory reads for 'a' and
'b'.
>>
>> Thoughts?
>
> I'm not sure if I got the details of how the tests work, but
wouldn't
> this allow false positives?
>
> Take this, for example:
>
> ; CHECK-PUSH:
> ; CHECK: memw(##a)
> ; CHECK-POP:
> ; CHECK: memw(##b)
>
> %0 = load i32* @a, align 4
> %1 = load i32* @b, align 4
>
> ; CHECK: memw(##a)
>
> %2 = load i32* @a, align 4
>
>
> My understanding is that this is supposed to detect:
> - 2 loads in any order, one from @a, the other from @b, followed by
> - 1 load from @a,
> that is, a total of 3 loads.
>
> At the same time I believe that it would positively match this code:
>   ... = memw(##b)
>   ... = memw(##a)
> i.e. only two loads.  The second load would match two different CHECKs.
>
> -K
>
You are correct. If the intent is to detect:

  * 2 loads in any order, one from @a, the other from @b, followed by
  * 1 load from @a,

the example does not do that. It matches additional sequences as well.

I don't believe the PUSH/POP functionality is expressive enough to 
handle this case. You would have to modify the test in some way or add 
more functionality to FileCheck to handle this.

Thinking of possible solutions ...

We could keep track of a "high water mark" and add a directive to set 
the position to it:

    ; CHECK-PUSH:
    ; CHECK: memw(##a)
    ; CHECK-POP:
    ; CHECK: memw(##b)
    ; CHECK-HWM:

    %0 = load i32* @a, align 4
    %1 = load i32* @b, align 4

    ; CHECK: memw(##a)

    %2 = load i32* @a, align 4

In this case CHECK-HWM (need a better name) will set the position to 
just after 'memw(##a)' or 'memw(##b)' whichever is later in the
file.

I've attached a new patch with support for the additional directive.

Matthew Curtis.

-- 
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum, hosted by The
Linux Foundation

-------------- next part --------------
An HTML attachment was scrubbed...
URL:
<http://lists.llvm.org/pipermail/llvm-dev/attachments/20120907/c61e1aaf/attachment.html>
-------------- next part --------------
diff --git a/utils/FileCheck/FileCheck.cpp b/utils/FileCheck/FileCheck.cpp
index 33f04ce..b6c4f51 100644
--- a/utils/FileCheck/FileCheck.cpp
+++ b/utils/FileCheck/FileCheck.cpp
@@ -50,11 +50,23 @@ NoCanonicalizeWhiteSpace("strict-whitespace",
 //===----------------------------------------------------------------------===//
 
 class Pattern {
+public:
+  enum MatchType {
+    MatchStr,
+    MatchCurrent,
+    MatchEndOfFile
+  };
+
+private:
   SMLoc PatternLoc;
 
-  /// MatchEOF - When set, this pattern only matches the end of file. This is
-  /// used for trailing CHECK-NOTs.
-  bool MatchEOF;
+  /// MatchType - When set to ...
+  ///   MatchStr, this pattern matches according to FixedStr or RegExStr.
+  ///   MatchCurrent, this pattern matches (the empty string at) the current
+  ///     position. This is used for CHECK-PUSHes without preceding CHECKs.
+  ///   MatchEndOfFile, this pattern only matches the end of file. This is used
+  ///     for trailing CHECK-NOTs.
+  enum MatchType Type;
 
   /// FixedStr - If non-empty, this pattern is a fixed string match with the
   /// specified fixed string.
@@ -77,7 +89,7 @@ class Pattern {
 
 public:
 
-  Pattern(bool matchEOF = false) : MatchEOF(matchEOF) { }
+  Pattern(enum MatchType t = MatchStr) : Type(t) { }
 
   bool ParsePattern(StringRef PatternStr, SourceMgr &SM);
 
@@ -285,9 +297,18 @@ bool Pattern::AddRegExToRegEx(StringRef RegexStr, unsigned
&CurParen,
 size_t Pattern::Match(StringRef Buffer, size_t &MatchLen,
                       StringMap<StringRef> &VariableTable) const {
   // If this is the EOF pattern, match it immediately.
-  if (MatchEOF) {
+  switch (Type) {
+  case MatchStr:
+    break;
+  case MatchCurrent:
+    MatchLen = 0;
+    return 0;
+  case MatchEndOfFile:
     MatchLen = 0;
     return Buffer.size();
+  default:
+    assert("Unknown match type");
+    break;
   }
 
   // If this is a fixed string pattern, just match it now.
@@ -447,6 +468,9 @@ struct CheckString {
   /// IsCheckNext - This is true if this is a CHECK-NEXT: directive (as opposed
   /// to a CHECK: directive.
   bool IsCheckNext;
+  int PushPos;
+  int PopPos;
+  SMLoc PopLoc;
 
   /// NotStrings - These are all of the strings that are disallowed from
   /// occurring between this match string and the previous one (or start of
@@ -454,7 +478,7 @@ struct CheckString {
   std::vector<std::pair<SMLoc, Pattern> > NotStrings;
 
   CheckString(const Pattern &P, SMLoc L, bool isCheckNext)
-    : Pat(P), Loc(L), IsCheckNext(isCheckNext) {}
+    : Pat(P), Loc(L), IsCheckNext(isCheckNext), PushPos(0), PopPos(0) {}
 };
 
 /// CanonicalizeInputFile - Remove duplicate horizontal space from the
specified
@@ -517,6 +541,8 @@ static bool ReadCheckFile(SourceMgr &SM,
   StringRef Buffer = F->getBuffer();
 
   std::vector<std::pair<SMLoc, Pattern> > NotMatches;
+  int NextCheckPopPos= 0;
+  SMLoc NextCheckPopLoc;
 
   while (1) {
     // See if Prefix occurs in the memory buffer.
@@ -531,6 +557,7 @@ static bool ReadCheckFile(SourceMgr &SM,
     // When we find a check prefix, keep track of whether we find CHECK: or
     // CHECK-NEXT:
     bool IsCheckNext = false, IsCheckNot = false;
+    bool IsCheckPush = false, IsCheckPop = false, IsCheckHWM = false;
 
     // Verify that the : is present after the prefix.
     if (Buffer[CheckPrefix.size()] == ':') {
@@ -543,6 +570,18 @@ static bool ReadCheckFile(SourceMgr &SM,
                memcmp(Buffer.data()+CheckPrefix.size(), "-NOT:", 5)
== 0) {
       Buffer = Buffer.substr(CheckPrefix.size()+6);
       IsCheckNot = true;
+    } else if (Buffer.size() > CheckPrefix.size()+6 &&
+               memcmp(Buffer.data()+CheckPrefix.size(), "-PUSH:", 6)
== 0) {
+      Buffer = Buffer.substr(CheckPrefix.size()+7);
+      IsCheckPush = true;
+    } else if (Buffer.size() > CheckPrefix.size()+5 &&
+               memcmp(Buffer.data()+CheckPrefix.size(), "-POP:", 5)
== 0) {
+      Buffer = Buffer.substr(CheckPrefix.size()+6);
+      IsCheckPop = true;
+    } else if (Buffer.size() > CheckPrefix.size()+5 &&
+               memcmp(Buffer.data()+CheckPrefix.size(), "-HWM:", 5)
== 0) {
+      Buffer = Buffer.substr(CheckPrefix.size()+6);
+      IsCheckHWM = true;
     } else {
       Buffer = Buffer.substr(1);
       continue;
@@ -555,6 +594,34 @@ static bool ReadCheckFile(SourceMgr &SM,
     // Scan ahead to the end of line.
     size_t EOL = Buffer.find_first_of("\n\r");
 
+    if (IsCheckPush) {
+      if (CheckStrings.empty()) {
+        CheckStrings.push_back(CheckString(Pattern(Pattern::MatchCurrent),
+                                          
SMLoc::getFromPointer(Buffer.data()),
+                                           false));
+      }
+      CheckStrings.back().PushPos++;
+      continue;
+    }
+
+    if (IsCheckPop) {
+      NextCheckPopLoc = SMLoc::getFromPointer(CheckPrefixStart);
+      NextCheckPopPos++;
+      continue;
+    }
+
+    if (IsCheckHWM) {
+      if (NextCheckPopPos) {
+        SM.PrintMessage(SMLoc::getFromPointer(CheckPrefixStart),
+                        SourceMgr::DK_Error,
+                        "found '"+CheckPrefix+"-HWM:'
without previous '"+
+                        CheckPrefix+ ": line");
+      }
+      NextCheckPopLoc = SMLoc::getFromPointer(CheckPrefixStart);
+      NextCheckPopPos = -1;
+      continue;
+    }
+
     // Remember the location of the start of the pattern, for diagnostics.
     SMLoc PatternLoc = SMLoc::getFromPointer(Buffer.data());
 
@@ -587,14 +654,22 @@ static bool ReadCheckFile(SourceMgr &SM,
     CheckStrings.push_back(CheckString(P,
                                        PatternLoc,
                                        IsCheckNext));
+    CheckStrings.back().PopPos= NextCheckPopPos;
+    CheckStrings.back().PopLoc= NextCheckPopLoc;
+    NextCheckPopPos= 0;
+    NextCheckPopLoc= SMLoc();
     std::swap(NotMatches, CheckStrings.back().NotStrings);
   }
 
   // Add an EOF pattern for any trailing CHECK-NOTs.
   if (!NotMatches.empty()) {
-    CheckStrings.push_back(CheckString(Pattern(true),
+    CheckStrings.push_back(CheckString(Pattern(Pattern::MatchEndOfFile),
                                        SMLoc::getFromPointer(Buffer.data()),
                                        false));
+    CheckStrings.back().PopPos= NextCheckPopPos;
+    CheckStrings.back().PopLoc= NextCheckPopLoc;
+    NextCheckPopPos= 0;
+    NextCheckPopLoc= SMLoc();
     std::swap(NotMatches, CheckStrings.back().NotStrings);
   }
 
@@ -686,10 +761,32 @@ int main(int argc, char **argv) {
   StringRef Buffer = F->getBuffer();
 
   const char *LastMatch = Buffer.data();
+  std::vector<StringRef> BufferStack;
+  StringRef BufferHighWaterMark = Buffer;
 
   for (unsigned StrNo = 0, e = CheckStrings.size(); StrNo != e; ++StrNo) {
     const CheckString &CheckStr = CheckStrings[StrNo];
 
+    if (CheckStr.PopPos == -1) {
+      if (Buffer.data() < BufferHighWaterMark.data()) {
+        Buffer = BufferHighWaterMark;
+        LastMatch = Buffer.data();
+      }
+    } else {
+      for (int i = CheckStr.PopPos; i; --i) {
+        if (BufferStack.empty()) {
+          SM.PrintMessage(CheckStr.PopLoc, SourceMgr::DK_Error,
+                          "attempting to
'"+CheckPrefix+"-POP:' without previous '"+
+                          CheckPrefix+ "-PUSH:'");
+          return 1;
+        }
+        if (Buffer.data() > BufferHighWaterMark.data())
+          BufferHighWaterMark = Buffer;
+        Buffer = BufferStack.back();
+        BufferStack.pop_back();
+      }
+    }
+
     StringRef SearchFrom = Buffer;
 
     // Find StrNo in the file.
@@ -756,6 +853,9 @@ int main(int argc, char **argv) {
     // the position after the match as the end of the last match.
     Buffer = Buffer.substr(MatchLen);
     LastMatch = Buffer.data();
+    for (int i= CheckStr.PushPos; i; --i) {
+      BufferStack.push_back(Buffer);
+    }
   }
 
   return 0;

On Fri, Sep 7, 2012 at 8:20 AM, Matthew Curtis <mcurtis at
codeaurora.org>wrote:
>  Hello all,
>
> For the hexagon target, we have a couple of tests that are failing due to
> variations in the order of checked text. In these cases the ordering is not
> directly relevant to the functionality being tested.
>
> For example:
>
> ; CHECK: memw(##a)
> ; CHECK: memw(##b)
>
> %0 = load i32* @a, align 4
> %1 = load i32* @b, align 4
>
> requires that the compiler emit the memory operations for 'a' and
'b' in
> that order, even though the intent of the test might simply be to ensure
> that each 'load' results in a memory read.
>
> I'd like to teach FileCheck to allow tests to be more tolerant of these
> incidental variations.
>
So, I'm not a huge fan of this, but for a strange reason.

Fundamentally, I agree with you that these ordering dependencies are
unfortunate in tests. However, I would make a few observations:

1) The order of instructions, unlike some things such as register
allocation's selection of registers, should ideally not vary much.
Currently it does more that we would like due to the inability to unit test
single pieces of the backend, but I don't think we should make FileCheck
better to cope with that, I think we should fix the underlying problem.

2) It is usually fairly obvious when two such checks don't actually have an
ordering constraint that is important, and where it isn't, a comment makes
the intent clear.

3) By checking the ordering, we gain at least some early detection of
non-determinism in the code generator. I definitely caught several bugs in
the block placement pass while I was working on it due to this very
"feature" of the test suite.


Given these points, the value tradeoff here of making FileCheck
*significantly* more complicated (and the tests significantly more complex
as well) versus the time savings updating test cases when the ordering
changes...

But maybe others who have spent more time hammering against these problems
in the backend have a different sense of the magnitude of the problem?

> The attached patch implements one possible solution. It introduces a
> position stack and a couple of directives:
>
>    - 'CHECK-PUSH:' pushes the current match position onto the
stack.
>    - 'CHECK-POP:' pops the top value off of the stack and uses it
to set
>    the current match position.
>
> The above test can now be re-written as:
>
> ; CHECK-PUSH:
> ; CHECK: memw(##a)
> ; CHECK-POP:
> ; CHECK: memw(##b)
>
> %0 = load i32* @a, align 4
> %1 = load i32* @b, align 4
>
> which handles either ordering of memory reads for 'a' and
'b'.
>
> Thoughts?
>
> Thanks,
> Matthew Curtis
>
> --
> Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum, hosted
by The Linux Foundation
>
>
> _______________________________________________
> LLVM Developers mailing list
> LLVMdev at cs.uiuc.edu         http://llvm.cs.uiuc.edu
> http://lists.cs.uiuc.edu/mailman/listinfo/llvmdev
>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL:
<http://lists.llvm.org/pipermail/llvm-dev/attachments/20120907/997dfc3e/attachment.html>

On 9/7/2012 4:08 PM, Chandler Carruth wrote:
> On Fri, Sep 7, 2012 at 8:20 AM, Matthew Curtis <mcurtis at
codeaurora.org
> <mailto:mcurtis at codeaurora.org>> wrote:
>
>     Hello all,
>
>     For the hexagon target, we have a couple of tests that are failing
>     due to variations in the order of checked text. In these cases the
>     ordering is not directly relevant to the functionality being tested.
>
>     For example:
>
>         ; CHECK: memw(##a)
>         ; CHECK: memw(##b)
>
>         %0 = load i32* @a, align 4
>         %1 = load i32* @b, align 4
>
>     requires that the compiler emit the memory operations for 'a'
and
>     'b' in that order, even though the intent of the test might
simply
>     be to ensure that each 'load' results in a memory read.
>
>     I'd like to teach FileCheck to allow tests to be more tolerant of
>     these incidental variations.
>
>
> So, I'm not a huge fan of this, but for a strange reason.
>
> Fundamentally, I agree with you that these ordering dependencies are 
> unfortunate in tests. However, I would make a few observations:
>
> 1) The order of instructions, unlike some things such as register 
> allocation's selection of registers, should ideally not vary much. 
> Currently it does more that we would like due to the inability to unit 
> test single pieces of the backend, but I don't think we should make 
> FileCheck better to cope with that, I think we should fix the 
> underlying problem.
>
> 2) It is usually fairly obvious when two such checks don't actually 
> have an ordering constraint that is important, and where it isn't, a 
> comment makes the intent clear.
>
> 3) By checking the ordering, we gain at least some early detection of 
> non-determinism in the code generator. I definitely caught several 
> bugs in the block placement pass while I was working on it due to this 
> very "feature" of the test suite.
>
>
> Given these points, the value tradeoff here of making FileCheck 
> *significantly* more complicated (and the tests significantly more 
> complex as well) versus the time savings updating test cases when the 
> ordering changes...
>
To be honest, I'm fairly new to LLVM and Clang so I don't have a good 
feel for how often this will come up. I've encountered two tests over 
the past month and a half that have regressed due to ordering changes. 
In one case the order varies based on target, so I would have to 
duplicate the test and enable/disable one or the other based on target. 
Not great, but if it's just one test probably acceptable.

As far as complexity, I didn't feel like the complexity added to 
FileCheck was that significant, though relative to how often the feature 
would (or should) get used perhaps it is. I was more concerned about 
adding 2-3 new directives to the user interface. In the end, that 
acceptable given that users would only see the additional complexity if 
they needed it.

If no one else feels like they would benefit much from this, I'll 
probably just wait and see how much of a maintenance burden this really 
is. If it continues to be an issue I can bring it back up to the community.
> But maybe others who have spent more time hammering against these 
> problems in the backend have a different sense of the magnitude of the 
> problem?
>
>
>     The attached patch implements one possible solution. It introduces
>     a position stack and a couple of directives:
>
>       * 'CHECK-PUSH:' pushes the current match position onto the
stack.
>       * 'CHECK-POP:' pops the top value off of the stack and uses
it
>         to set the current match position.
>
>     The above test can now be re-written as:
>
>         ; CHECK-PUSH:
>         ; CHECK: memw(##a)
>         ; CHECK-POP:
>         ; CHECK: memw(##b)
>
>         %0 = load i32* @a, align 4
>         %1 = load i32* @b, align 4
>
>     which handles either ordering of memory reads for 'a' and
'b'.
>
>     Thoughts?
>
>     Thanks,
>     Matthew Curtis
>
>     -- 
>     Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
hosted by The Linux Foundation
>
>
>     _______________________________________________
>     LLVM Developers mailing list
>     LLVMdev at cs.uiuc.edu <mailto:LLVMdev at cs.uiuc.edu>
>     http://llvm.cs.uiuc.edu
>     http://lists.cs.uiuc.edu/mailman/listinfo/llvmdev
>
>

-- 
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum, hosted by The
Linux Foundation

-------------- next part --------------
An HTML attachment was scrubbed...
URL:
<http://lists.llvm.org/pipermail/llvm-dev/attachments/20120910/cb0c4a50/attachment.html>

On 9/7/2012 4:08 PM, Chandler Carruth wrote:
>
> Fundamentally, I agree with you that these ordering dependencies are
> unfortunate in tests. However, I would make a few observations:
I disagree.  A testcase should test what it was meant to test, and 
nothing else.  Instruction ordering, for example, is not something that 
we can expect to remain the same.  Various optimizations can affect it 
(changes in the scheduler being the most obvious case), causing spurious 
failures that would need to be investigated and addressed.

As a matter of fact, we should make an effort to make our tests 
independent of any unrelated (but valid) optimizations that may be added 
at some point in the future.  I realize that this is not always possible 
to achieve completely, but the more precise we get with the tests, the 
better off we will be in the long run.

I don't think that FileCheck is modified as frequently as the compiler 
itself, so the added complexity (which apparently is not large in this 
case) is not likely to add a significant recurring cost.

-K

-- 
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum, 
hosted by The Linux Foundation