Page MenuHomePhabricator

LSR: Check more intrinsic pointer operands
ClosedPublic

Authored by arsenm on Jan 30 2017, 12:05 PM.

Diff Detail

Event Timeline

arsenm created this revision.Jan 30 2017, 12:05 PM
hfinkel edited edge metadata.Mar 15 2017, 2:46 PM

I'd rather not embed knowledge of all relevant target-specific intrinsics in this file. Can you use the TTI->getTgtMemIntrinsic interface instead?

arsenm updated this revision to Diff 91955.Mar 15 2017, 4:16 PM

Implement getTgtMemIntrinsic and use it. I'm not sure this is correct because it should probably be reported as atomic, but IsSimple's only purpose seems to be to break an assert in EarlyCSE

hfinkel added inline comments.Mar 15 2017, 7:49 PM
lib/Target/AMDGPU/AMDGPUTargetTransformInfo.cpp
299

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.

lib/Transforms/Scalar/LoopStrengthReduce.cpp
834–835

This is fine for prefetch, but can you replace the others with a call to TTI.getTgtMemIntrinsic also?

arsenm updated this revision to Diff 92038.Mar 16 2017, 12:45 PM

Fix missed place, replace IsSimple

arsenm updated this revision to Diff 124687.Nov 28 2017, 7:29 PM

Update intrinsics in tests

qcolombet accepted this revision.Dec 11 2017, 10:54 AM
qcolombet added a subscriber: qcolombet.

LGTM for the generic part.

This revision is now accepted and ready to land.Dec 11 2017, 10:54 AM
arsenm closed this revision.Dec 11 2017, 1:39 PM

r320424