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

[Nikita Popov via Phabricator via llvm-commits]

nikic added inline comments.


================
Comment at: llvm/include/llvm/IR/InstrTypes.h:2334
-  }
-
   /// Determine whether the return value has the given attribute. Supports
----------------
jdoerfert wrote:
> IS this support moved somewhere? Do we support this for in-tree code? 
This is moved into getMemoryEffects(), which properly handles operand bundle effects. (And things like doesNotAcessMemory() use getMemoryEffects() internally, so they also handle this correctly.)


================
Comment at: llvm/lib/Bitcode/Reader/BitcodeReader.cpp:1722
-  case Attribute::InaccessibleMemOnly:         return 1ULL << 49;
-  case Attribute::InaccessibleMemOrArgMemOnly: return 1ULL << 50;
   case Attribute::SwiftSelf:       return 1ULL << 51;
----------------
jdoerfert wrote:
> Should we keep a comment to make sure the numbersare not reused?
This attribute encoding is no longer used (only exists for auto-upgrade), but sure, a comment won't hurt...


================
Comment at: llvm/lib/Transforms/Scalar/SCCP.cpp:604
+              F.getContext(),
+              Attribute::getWithMemoryEffects(F.getContext(), ME));
+        };
----------------
jdoerfert wrote:
> This is a little more pessimistic than the original version but I doubt we should be too concerned. We should rather revisit this as we introduce the "globals"/"constants" category. Then we can also remain the read/write-only effect for them (or above for Other).
I've tweaked this to copy the ModRef from ArgMem. Dropping the read/write info here wasn't intentional...

And yes, we can do better here after splitting "Other".


================
Comment at: llvm/lib/Transforms/Utils/CodeExtractor.cpp:904
       switch (Attr.getKindAsEnum()) {
       // Those attributes cannot be propagated safely. Explicitly list them
       // here so we get a warning if new attributes are added.
----------------
jdoerfert wrote:
> Don't we have to add memory?
It's already there, right before the continue. This was added in the previous patch.


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

https://reviews.llvm.org/D135780