[PATCH] D135780: [IR] Switch everything to use memory attribute

[Johannes Doerfert via Phabricator via llvm-commits]

jdoerfert added inline comments.


================
Comment at: llvm/lib/Transforms/IPO/AttributorAttributes.cpp:7370
+        /*ForceReplace*/ true);
+  }
+
----------------
Can you leave a TODO or two here. We need to de-duplicate the code with the copy above and probably rethink what we emit, e.g., if we could emit better information.


================
Comment at: llvm/lib/Transforms/IPO/AttributorAttributes.cpp:7683
+        }
+        continue;
       }
----------------
This part (I think) broke a test, see below.

I'll assume the `|=` works as required here (though I find it counter-intuitive).
The dropping of the attributes feels complex nevertheless. Can't we just drop the memory attribute all together?
Can we verify the attribute is dropped from the IR?


================
Comment at: llvm/lib/Transforms/IPO/AttributorAttributes.cpp:7734
+        Attribute::getWithMemoryEffects(IRP.getAnchorValue().getContext(), ME),
+        /*ForceReplace*/ true);
   }
----------------
Probably again a TODO.


================
Comment at: llvm/test/Transforms/Attributor/memory_locations.ll:709
   ret void
 }
 ;.
----------------
This is a test to catch the instcombine + argmemonly problem and we broke it.
The TUNIT case is correct but the CGSCC case is not. Both line 685 and 701 are wrong.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D135780/new/

https://reviews.llvm.org/D135780