[PATCH] D29294: LSR: Check more intrinsic pointer operands

[Hal Finkel via Phabricator via llvm-commits]

hfinkel added inline comments.


================
Comment at: lib/Target/AMDGPU/AMDGPUTargetTransformInfo.cpp:174
+
+    // FIXME: Should this be false since atomic? EarlyCSE asserts if this is
+    // ever set.
----------------
Yes, it is. It seems to be there to remind us to split this apart when we have some occasion to set it to true. The assertions in EarlyCSE look like this:

    bool isAtomic() const {
      if (IsTargetMemInst) {
        assert(Info.IsSimple && "need to refine IsSimple in TTI");
        return false;
      }
      return Inst->isAtomic();
    }
    bool isUnordered() const {
      if (IsTargetMemInst) {
        assert(Info.IsSimple && "need to refine IsSimple in TTI");
        return true;
      }

Is looks like we should split IsSimple into:

  AtomicOrdering Ordering (initialized to AtomicOrdering::NotAtomic)
  bool IsVolatile (initialized to false)

And then in EarlyCSE return IsVolatile for isVolatile(), return Ordering != AtomicOrdering::NotAtomic for isAtomic(), and return (Ordering == AtomicOrdering::NotAtomic || Ordering == AtomicOrdering::Unordered) && !IsVolatile for isUnordered(). This is what we do for loads and stores.

Can you just do that and then we can get rid of the asserts? While it's true that lying to EarlyCSE this way will be okay so long as you don't also implement getOrCreateResultFromMemIntrinsic, I'd much rather we just take care of this now and not worry about leaving semantically-incorrect information.



================
Comment at: lib/Transforms/Scalar/LoopStrengthReduce.cpp:815-816
+    switch (II->getIntrinsicID()) {
+    case Intrinsic::prefetch:
+    case Intrinsic::amdgcn_atomic_inc:
+    case Intrinsic::amdgcn_atomic_dec:
----------------
This is fine for prefetch, but can you replace the others with a call to TTI.getTgtMemIntrinsic also?


https://reviews.llvm.org/D29294