PMADDWD can help improve 8/16 bit integer mutliply-add operation performance for cases like:
for (int i = 0; i < count; i++)
a += x[i] * y[i];
Details
Details
Diff Detail
Diff Detail
- Build Status
Buildable 5300 Build 5300: arc lint + arc unit
Event Timeline
| lib/Target/X86/X86ISelLowering.cpp | ||
|---|---|---|
| 34588–34599 | Maybe use std::swap, so that Op0 and Op1 are unnecessary.
| |
| 34605–34607 | Just realize maybe we need to prove no wrap before use PMADDUBSW? It is possible that i8*i8+i8*i8 will overflow for i16 and PMADDUBSW will generate saturate result. However, the original i32 operations will not have overflow issue. | |
Comment Actions
remove the support for PMADDUBSW as it cannot handle overflow case.
| lib/Target/X86/X86ISelLowering.cpp | ||
|---|---|---|
| 34605–34607 | Good catch! Looks like we should not use compiler to generate PMADDUBSW directly. Patch updated. | |
| test/CodeGen/X86/madd.ll | ||
|---|---|---|
| 25 | The test can be simpler by removing the preheader and exit. just like what test/CodeGen/X86/sad.ll does. It will be easier if it is needed to extend the test and check different kinds of element types or different vector lengths. | |
Comment Actions
Thanks for working on this patch. Regarding support for PMADDUBSW, can we match something like the following?
for (int i = 0; i < count; i++) {
a = saturate(a + x[i] * y[i]);
}Comment Actions
I suggest we leave the PMADDUBSW discussion for a separate patch.
Some minor comments inline.
| lib/Target/X86/X86ISelLowering.cpp | ||
|---|---|---|
| 34600 | Since this version always uses PMADDWD, we no longer care about Mode at all, and this entire if can go away. | |
| 34630 | Isn't Madd.getSimpleValueType() always MVT::i32? | |
| 34630 | Just a sanity check - this is correct irrespective of whether the initial value for the reduction phi is 0 or not, right? | |
| 34732 | Bikeshedding: maybe call this combineLoopMAddPattern() to match the other name? | |
| 34732 | Sad -> Madd? | |
Comment Actions
I'm not aware of such a builtin and my snippet above was more of pseudo-code.
In c++ you may implement saturation as:
int sat_sint16(int x) {
return std::min(32767, std::max(-32768, x));
}AFAIK, the loop vectorizer will not vectorize the reduction for PMADDUBSW, so i agree with @mkuper to do this in a different patch,
Maybe use std::swap, so that Op0 and Op1 are unnecessary.
MulOp = N->getOperand(0);
Phi = N->getOperand(1);
if (MulOp.getOpcode() != ISD::MUL) {
std::swap(MulOp, Phi);
if (MulOp.getOpcode() != ISD::MUL)
return SDValue();
}