[GUFA] Fix packed field filtering (#5652)

Technically we need to filter both before and after combining, that is,
if a location's contents will be filtered by F() then if the new contents
are x and old contents y then we need to end up with
F(F(x) U F(y)). That is, filtering before is necessary to ensure the union
of content does not end up unnecessarily large, and the filtering
after is necessary to ensure the final result is properly filtered to fit.
(If our representation were perfect then this would not be needed, but
it is not, as the union of two exact types can end up as a very large
cone, for example.)

For efficiency we have been filtering afterwards. But that is not enough
for packed fields, it turns out, where we must filter before. If we don't,
then if inputs 0 and 0x100 arrive to an i8 field then combining them
we get "unknown integer" (which is then filtered by 0xff, but it's too
late). By filtering before, the actual values are both 0 and we end up
with that as the only possible value.

It turns out that filtering before is enough for such fields, so do only
that.

1 files changed, 41 insertions, 6 deletions

diff --git a/src/ir/possible-contents.cpp b/src/ir/possible-contents.cpp
index d63830c40..6c6c1b97e 100644
--- a/src/ir/possible-contents.cpp
+++ b/src/ir/possible-contents.cpp
@@ -1695,6 +1695,34 @@ bool Flower::updateContents(LocationIndex locationIndex,
   std::cout << '\n';
 #endif
 
+  auto location = getLocation(locationIndex);
+
+  // Handle special cases: Some locations can only contain certain contents, so
+  // filter accordingly. In principle we need to filter both before and after
+  // combining with existing content; filtering afterwards is obviously
+  // necessary as combining two things will create something larger than both,
+  // and our representation has limitations (e.g. two different ref types will
+  // result in a cone, potentially a very large one). Filtering beforehand is
+  // necessary for the a more subtle reason: consider a location that contains
+  // an i8 which is sent a 0 and then 0x100. If we filter only after, then we'd
+  // combine 0 and 0x100 first and get "unknown integer"; only by filtering
+  // 0x100 to 0 beforehand (since 0x100 & 0xff => 0) will we combine 0 and 0 and
+  // not change anything, which is correct.
+  //
+  // For efficiency reasons we aim to only filter once, depending on the type of
+  // filtering. Most can be filtered a single time afterwards, while for data
+  // locations, where the issue is packed integer fields, it's necessary to do
+  // it before as we've mentioned, and also sufficient (see details in
+  // filterDataContents).
+  if (auto* dataLoc = std::get_if<DataLocation>(&location)) {
+    filterDataContents(newContents, *dataLoc);
+#if defined(POSSIBLE_CONTENTS_DEBUG) && POSSIBLE_CONTENTS_DEBUG >= 2
+    std::cout << "  pre-filtered contents:\n";
+    newContents.dump(std::cout, &wasm);
+    std::cout << '\n';
+#endif
+  }
+
   contents.combine(newContents);
 
   if (contents.isNone()) {
@@ -1730,9 +1758,7 @@ bool Flower::updateContents(LocationIndex locationIndex,
     return worthSendingMore;
   }
 
-  // Handle special cases: Some locations can only contain certain contents, so
-  // filter accordingly.
-  auto location = getLocation(locationIndex);
+  // Handle filtering (see comment earlier, this is the later filtering stage).
   bool filtered = false;
   if (auto* exprLoc = std::get_if<ExpressionLocation>(&location)) {
     // TODO: Replace this with specific filterFoo or flowBar methods like we
@@ -1743,9 +1769,6 @@ bool Flower::updateContents(LocationIndex locationIndex,
   } else if (auto* globalLoc = std::get_if<GlobalLocation>(&location)) {
     filterGlobalContents(contents, *globalLoc);
     filtered = true;
-  } else if (auto* dataLoc = std::get_if<DataLocation>(&location)) {
-    filterDataContents(contents, *dataLoc);
-    filtered = true;
   }
 
   // Check if anything changed after filtering, if we did so.
@@ -1999,6 +2022,18 @@ void Flower::filterDataContents(PossibleContents& contents,
       //      value reach.
       contents = PossibleContents::fromType(contents.getType());
     }
+    // Given that the above only (1) turns an i32 into a masked i32 or (2) turns
+    // anything else into an unknown i32, this is safe to run as pre-filtering,
+    // that is, before we combine contents, since
+    //
+    //  (a) two constants are ok as masking is distributive,
+    //        (x & M) U (y & M)  ==  (x U y) & M
+    //  (b) if one is a constant and the other is not then
+    //        (x & M) U ?  ==  ?  ==  (x U ?)  ==  (x U ?) & M
+    //      (where ? is an unknown i32)
+    //  (c) and if both are not constants then likewise we always end up as an
+    //      unknown i32
+    //
   }
 }