[PATCH] D11678: [CodeGen] Fixes *absdiff* intrinsic: LangRef doc/test case improvement and corresponding code change
Michael Zolotukhin
mzolotukhin at apple.com
Fri Jul 31 13:26:09 PDT 2015
mzolotukhin requested changes to this revision.
mzolotukhin added a comment.
This revision now requires changes to proceed.
Hi Shahid,
Please find some comments inline:
================
Comment at: docs/LangRef.rst:10387-10390
@@ -10386,6 +10386,6 @@
%sub = sub nsw <4 x i32> %a, %b
- %ispos = icmp sgt <4 x i32> %sub, <i32 -1, i32 -1, i32 -1, i32 -1>
+ %ispos = icmp sge <4 x i32> %sub, zeroinitializer
%neg = sub nsw <4 x i32> zeroinitializer, %sub
%1 = select <4 x i1> %ispos, <4 x i32> %sub, <4 x i32> %neg
----------------
What's the difference between `llvm.uabsdiff` and `llvm.sabsdiff` then?
================
Comment at: lib/CodeGen/SelectionDAG/LegalizeVectorOps.cpp:737
@@ -736,5 +736,3 @@
TLI.getSetCCResultType(DAG.getDataLayout(), *DAG.getContext(), VT), Tmp2,
- DAG.getConstant(0, dl, VT),
- DAG.getCondCode(Op->getOpcode() == ISD::SABSDIFF ? ISD::SETLT
- : ISD::SETULT));
+ DAG.getConstant(0, dl, VT), DAG.getCondCode(ISD::SETGE));
Tmp1 = DAG.getNode(ISD::VSELECT, dl, VT, Tmp4, Tmp1, Tmp2);
----------------
AFAIU, this should be `ISD::SETLE`.
================
Comment at: test/CodeGen/X86/absdiff_128.ll:150-152
@@ +149,5 @@
+; CHECK-DAG: psubd %xmm1, [[SRC:%xmm[0-9]+]]
+; CHECK: pxor
+; CHECK-DAG: pxor %xmm3, [[DST:%xmm[0-9]+]]
+; CHECK-NEXT: psubd [[SRC]], [[DST]]
+; CHECK-DAG: pcmpgtd %xmm3, [[SRC:%xmm[0-9]+]]
----------------
This `CHECK-DAG` doesn't make much sense, since it's limited by `CHECK` and `CHECK-NEXT` from both sides.
Moreover, I think the right way to make the tests less bristle is to not check for everything, but just look for key instructions.
For example, we definitely expect to see `psubd`, then, maybe after several other instructions, we want to see `pcmpgt`, then we want to see `pand`, `pandn`, and `por`. Thus, I'd write this test something like this:
```
CHECK: psubd
CHECK: pcmpgt
CHECK-DAG: pand // BTW, why do you have two `pandn` here?
CHECK-DAG: pandn
CHECK: por
CHECK: ret
```
================
Comment at: test/CodeGen/X86/absdiff_128.ll:206
@@ +205,3 @@
+; CHECK-NEXT: pcmpgtd
+; CHECK-NEXT: pshufd {{.*}} # xmm5 = xmm4[0,0,2,2]
+; CHECK-NEXT: pcmpeqd
----------------
If we don't want to match any specific register here, we need to get rid of comments `# xmm5 = xmm4...` too.
================
Comment at: test/CodeGen/X86/absdiff_256.ll:33
@@ +32,3 @@
+; CHECK-DAG: psubd %xmm6, [[SRC:%xmm[0-9]+]]
+; CHECK-DAG: pxor %xmm5, [[DST:%xmm[0-9]+]]
+; CHECK-NEXT: psubd [[SRC]], [[DST]]
----------------
This is still fragile.
Imagine that register allocator for some strange reason begins to use `xmm5` instead of `xmm6` and vice versa - this test will immediately fail.
Also, if you want to match `pxor %xmmN, %xmmN`, the correct way to write the regexp for it would be:
```
pxor [[SOMENAME:%xmm[0-9]+]], [[SOMENAME]]
```
This will ensure that `pxor` operates on the same register.
Repository:
rL LLVM
http://reviews.llvm.org/D11678
More information about the llvm-commits
mailing list