Please use GitHub pull requests for new patches. Avoid migrating existing patches. Phabricator shutdown timeline
Changeset View
Standalone View
lib/Analysis/InstructionSimplify.cpp
| Show First 20 Lines • Show All 4,934 Lines • ▼ Show 20 Lines | static Value *simplifyUnaryIntrinsic(Function *F, Value *Op0, | ||||
| case Intrinsic::log: | case Intrinsic::log: | ||||
| // log(exp(x)) -> x | // log(exp(x)) -> x | ||||
| if (Q.CxtI->hasAllowReassoc() && | if (Q.CxtI->hasAllowReassoc() && | ||||
| match(Op0, m_Intrinsic<Intrinsic::exp>(m_Value(X)))) return X; | match(Op0, m_Intrinsic<Intrinsic::exp>(m_Value(X)))) return X; | ||||
| break; | break; | ||||
| case Intrinsic::log2: | case Intrinsic::log2: | ||||
| // log2(exp2(x)) -> x | // log2(exp2(x)) -> x | ||||
| if (Q.CxtI->hasAllowReassoc() && | if (Q.CxtI->hasAllowReassoc() && | ||||
| match(Op0, m_Intrinsic<Intrinsic::exp2>(m_Value(X)))) return X; | (match(Op0, m_Intrinsic<Intrinsic::exp2>(m_Value(X))) || | ||||
| match(Op0, m_Intrinsic<Intrinsic::pow>(m_SpecificFP(2.0), | |||||
lebedev.ri: Hmm, do we want to canonicalize `Intrinsic::pow(2.0, x)` to `exp2(x)` instead ? | |||||
QuolykAuthorUnsubmitted 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 ActionsIt'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 ActionsThere is a counter-argument to that: (that i have thought of after asking, but did not post) I.e. is there some situation where we might end up with simplifyUnaryIntrinsic() being called 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 ActionsInstsimplify 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 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
Yep, it's a good idea to keep patches small.
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… | |||||
| m_Value(X))))) return X; | |||||
| break; | |||||
| case Intrinsic::log10: | |||||
| // log10(pow(10.0, x)) -> x | |||||
| if (Q.CxtI->hasAllowReassoc() && | |||||
| match(Op0, m_Intrinsic<Intrinsic::pow>(m_SpecificFP(10.0), | |||||
| m_Value(X)))) return X; | |||||
| break; | break; | ||||
| default: | default: | ||||
| break; | break; | ||||
| } | } | ||||
| return nullptr; | return nullptr; | ||||
| } | } | ||||
| ▲ Show 20 Lines • Show All 517 Lines • Show Last 20 Lines | |||||
Hmm, do we want to canonicalize Intrinsic::pow(2.0, x) to exp2(x) instead ?