Page MenuHomePhabricator

Please use GitHub pull requests for new patches. Avoid migrating existing patches. Phabricator shutdown timeline

Differential D41940 Diff 135632 lib/Analysis/InstructionSimplify.cpp

Changeset View

Standalone View

View Options

lib/Analysis/InstructionSimplify.cpp

Show First 20 Lines • Show All 4,524 Lines • ▼ Show 20 Lines for (unsigned I = 0, E = ConstMask->getType()->getVectorNumElements(); I != E;
if (auto *MaskElt = ConstMask->getAggregateElement(I)) if (auto *MaskElt = ConstMask->getAggregateElement(I))
if (MaskElt->isNullValue() || isa<UndefValue>(MaskElt)) if (MaskElt->isNullValue() || isa<UndefValue>(MaskElt))
continue; continue;
return false; return false;
} }
return true; return true;
} }
template <typename IterTy> template <typename IterTy>
lebedev.riUnsubmitted
Not Done
ReplyInline Actions

Hmm, do we want to canonicalize Intrinsic::pow(2.0, x) to exp2(x) instead ?

lebedev.ri: Hmm, do we want to canonicalize `Intrinsic::pow(2.0, x)` to `exp2(x)` instead ?
QuolykAuthorUnsubmitted
Done
ReplyInline Actions

If Intrinsic::pow(2.0, x) to exp2(x) transform is useful somewhere else or you have strong opinion about it we do. Otherwise I don't see the reason for this.

Quolyk: If `Intrinsic::pow(2.0, x)` to `exp2(x)` transform is useful somewhere else or you have strong…
spatelUnsubmitted
Not Done
ReplyInline Actions

It's a specialized form of the more general math function, so we can assume it has a faster implementation. I think we should add that fold and reduce this patch.

spatel: It's a specialized form of the more general math function, so we can assume it has a faster…
lebedev.riUnsubmitted
Not Done
ReplyInline Actions

There is a counter-argument to that: (that i have thought of after asking, but did not post)
do we *know* that instcombine would have always been run after such an Intrinsic call was added,
and before this simplifyUnaryIntrinsic() was called?

I.e. is there some situation where we might end up with simplifyUnaryIntrinsic() being called
*before* instcombine had a chance to do that canonicalization?
If yes, do we care?

lebedev.ri: There is a counter-argument to that: (that i have thought of after asking, but did not post) do…
spatelUnsubmitted
Not Done
ReplyInline Actions

Instsimplify may be used independently of instcombine by other passes, so yes, it is possible that the non-canonical form will be seen here. It's not generally a concern with the typical optimization pipeline since we run instcombine...a lot.

So the full solution is: leave this patch as-is (handle both forms) and add the instcombine too.

spatel: Instsimplify may be used independently of instcombine by other passes, so yes, it is possible…
QuolykAuthorUnsubmitted
Done
ReplyInline Actions

I suppose this transform needs to be done in a separate patch. Do I need to commit tests before this patch as in D41342?

Quolyk: I suppose this transform needs to be done in a separate patch. Do I need to commit tests before…
lebedev.riUnsubmitted
Not Done
ReplyInline Actions

I suppose this transform needs to be done in a separate patch.

Yep, it's a good idea to keep patches small.

Do I need to commit tests before this patch as in D41342?

It really helps with review.

lebedev.ri: > I suppose this transform needs to be done in a separate patch. Yep, it's a good idea to keep…
static Value *SimplifyIntrinsic(Function *F, IterTy ArgBegin, IterTy ArgEnd, static Value *SimplifyIntrinsic(Function *F, IterTy ArgBegin, IterTy ArgEnd,
const SimplifyQuery &Q, unsigned MaxRecurse) { const SimplifyQuery &Q, unsigned MaxRecurse) {
Intrinsic::ID IID = F->getIntrinsicID(); Intrinsic::ID IID = F->getIntrinsicID();
unsigned NumOperands = std::distance(ArgBegin, ArgEnd); unsigned NumOperands = std::distance(ArgBegin, ArgEnd);
// Unary Ops // Unary Ops
if (NumOperands == 1) { if (NumOperands == 1) {
// Perform idempotent optimizations // Perform idempotent optimizations
▲ Show 20 Lines • Show All 43 Lines • ▼ Show 20 Lines case Intrinsic::log: {
if (Q.CxtI->hasAllowReassoc() && if (Q.CxtI->hasAllowReassoc() &&
match(IIOperand, m_Intrinsic<Intrinsic::exp>(m_Value(X)))) match(IIOperand, m_Intrinsic<Intrinsic::exp>(m_Value(X))))
return X; return X;
return nullptr; return nullptr;
} }
case Intrinsic::log2: { case Intrinsic::log2: {
// log2(exp2(x)) -> x // log2(exp2(x)) -> x
if (Q.CxtI->hasAllowReassoc() && if (Q.CxtI->hasAllowReassoc() &&
match(IIOperand, m_Intrinsic<Intrinsic::exp2>(m_Value(X)))) { (match(IIOperand, m_Intrinsic<Intrinsic::exp2>(m_Value(X))) ||
match(IIOperand, m_Intrinsic<Intrinsic::pow>(m_SpecificFP(2.0),
m_Value(X)))))
return X; return X;
return nullptr;
} }
case Intrinsic::log10: {
// log10(pow(10.0, x)) -> x
if (Q.CxtI->hasAllowReassoc() &&
match(IIOperand, m_Intrinsic<Intrinsic::pow>(m_SpecificFP(10.0),
m_Value(X))))
return X;
return nullptr; return nullptr;
} }
default: default:
return nullptr; return nullptr;
} }
} }
// Binary Ops // Binary Ops
if (NumOperands == 2) { if (NumOperands == 2) {
▲ Show 20 Lines • Show All 404 Lines • Show Last 20 Lines