Details
- Reviewers
• tstellarAMD hfinkel qcolombet
Diff Detail
Event Timeline
I'd rather not embed knowledge of all relevant target-specific intrinsics in this file. Can you use the TTI->getTgtMemIntrinsic interface instead?
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
| lib/Target/AMDGPU/AMDGPUTargetTransformInfo.cpp | ||
|---|---|---|
| 174 | 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 | ||
| 815–816 | This is fine for prefetch, but can you replace the others with a call to TTI.getTgtMemIntrinsic also? | |
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:
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.