Subzero: Fix over-aggressive bool folding.

src/IceTargetLoweringX86BaseImpl.h

Index: src/IceTargetLoweringX86BaseImpl.h
diff --git a/src/IceTargetLoweringX86BaseImpl.h b/src/IceTargetLoweringX86BaseImpl.h
index 4927e170598d69eded4aa851733b30ae89b114c8..7dde1e60b538684a0981213b1f99d44fa6e43cc9 100644
--- a/src/IceTargetLoweringX86BaseImpl.h
+++ b/src/IceTargetLoweringX86BaseImpl.h
@@ -151,6 +151,7 @@ private:
     return Element != Producers.end() && Element->second.Instr != nullptr;
   }
   void setInvalid(SizeT VarNum) { Producers[VarNum].Instr = nullptr; }
+  void invalidateProducersOnStore(const Inst *Instr);
   /// Producers maps Variable::Number to a BoolFoldingEntry.
   CfgUnorderedMap<SizeT, BoolFoldingEntry<Traits>> Producers;
 };
@@ -252,10 +253,12 @@ bool BoolFolding<Traits>::isValidFolding(
 template <typename Traits> void BoolFolding<Traits>::init(CfgNode *Node) {
   Producers.clear();
   for (Inst &Instr : Node->getInsts()) {
+    if (Instr.isDeleted())

[Eric Holk, 2016/04/21 22:09:43]

As a future CL, it would probably be nice to add a non-deleted instructions
iterator (or maybe active or live instructions instead of calling them
non-deleted) so we don't have to remember to do this check.

[Jim Stichnoth, 2016/04/21 22:50:29]

That's a great idea.

+      continue;
+    invalidateProducersOnStore(&Instr);
     // Check whether Instr is a valid producer.
     Variable *Var = Instr.getDest();
-    if (!Instr.isDeleted() // only consider non-deleted instructions
-        && Var             // only instructions with an actual dest var
+    if (Var // only consider instructions with an actual dest var
         && Var->getType() == IceType_i1          // only bool-type dest vars
         && getProducerKind(&Instr) != PK_None) { // white-listed instructions
       Producers[Var->getIndex()] = BoolFoldingEntry<Traits>(&Instr);
@@ -338,6 +341,38 @@ void BoolFolding<Traits>::dump(const Cfg *Func) const {
   }
 }
 
+/// If the given instruction has potential memory side effects (e.g. store, rmw,
+/// or a call instruction with potential memory side effects), then we must not
+/// allow a pre-store Producer instruction with memory operands to be folded
+/// into a post-store Consumer instruction.  If this is detected, the Producer
+/// is invalidated.
+///
+/// We use the Producer's IsLiveOut field to determine whether any potential
+/// Consumers come after this store instruction.  If IsLiveOut has already been
+/// set to false, then we know the folding is safe from the store instruction.

[John, 2016/04/21 22:50:02]

"If IsLiveOut has already been set to false, then we know the folding is safe
from the store instruction." why?

[Jim Stichnoth, 2016/04/21 23:31:35]

Done, hopefully it's more explanatory now.

+template <typename Traits>
+void BoolFolding<Traits>::invalidateProducersOnStore(const Inst *Instr) {
+  if (!Instr->isMemoryWrite())
+    return;
+  for (auto &ProducerPair : Producers) {
+    if (!ProducerPair.second.IsLiveOut)
+      continue;
+    Inst *PInst = ProducerPair.second.Instr;
+    if (PInst == nullptr)
+      continue;
+    bool HasMemOperand = false;
+    const SizeT SrcSize = PInst->getSrcSize();
+    for (SizeT I = 0; !HasMemOperand && I < SrcSize; ++I) {

[John, 2016/04/21 22:50:02]

why not breaking instead? I would expect llvm to efficiently lower

for (var = init; var < const; ++var)

but I would be surprised if it managed to do anything decent with moar
complicated fors...

[Jim Stichnoth, 2016/04/21 23:31:35]

Done.

+      if (llvm::isa<typename Traits::X86OperandMem>(PInst->getSrc(I))) {
+        HasMemOperand = true;
+      }
+    }
+    if (!HasMemOperand)
+      continue;
+    setInvalid(ProducerPair.first);
+  }
+}
+
 template <typename TraitsType>
 void TargetX86Base<TraitsType>::initNodeForLowering(CfgNode *Node) {
   FoldingInfo.init(Node);