This is an archive of the discontinued LLVM Phabricator instance.

[Codegen] Added intrinsics 'absdiff' and corresponding SDNodes for absolute difference operation
ClosedPublic

Authored by ashahid on Jul 1 2015, 4:15 AM.

Details

Summary

This adds new intrinsics "*absdiff" for absolute difference ops to facilitate efficient code generation for "sum of absolute differences" operation.
The patch also contains the introduction of corresponding SDNodes and basic legalization support.Sanity of the generated code is tested on X86.

This is 1st of the three patches.

Diff Detail

Event Timeline

ashahid updated this revision to Diff 28854.Jul 1 2015, 4:15 AM
ashahid retitled this revision from to [Codegen] Added intrinsics 'absdiff' and corresponding SDNodes for absolute difference operation.
ashahid updated this object.
ashahid edited the test plan for this revision. (Show Details)
ashahid added reviewers: jmolloy, hfinkel, rengolin.
ashahid set the repository for this revision to rL LLVM.
ashahid added a subscriber: Unknown Object (MLST).
jmolloy edited edge metadata.Jul 6 2015, 4:11 AM

Apart from the comments I've given, this looks fine by me but please can you get the OK of someone like Hal or Chandler before committing, as this changes LangRef?

docs/LangRef.rst
10355

Slight re-Englishing: "... used during *the* code generation...", "They are generated by compiler passes such as the Loop and SLP vectorizers."

I'd also suggest adding an explanation of what that means for users, something like "The expectation is that frontends should not need to generate these intrinsics themselves."

include/llvm/IR/Intrinsics.td
610

These should be "LLVMMatchType<0>", not llvm_anyvector_ty. This enforces all arguments are of the same type as the result.

Hi James,

Thanks for your comments, will update the patch accordingly.

Hal / Chandler,
Please have a look, awaiting your comment if any.

Regards,
Shahid

bruno added a subscriber: bruno.Jul 6 2015, 12:18 PM

Hi,

Very nice. I wonder what are your plans regarding the ISD node for the horizontal sum part? It would be great if it's done in a way that we can also use it in the expansion of vector population count, as discussed in http://lists.cs.uiuc.edu/pipermail/llvm-commits/Week-of-Mon-20150601/279536.html

Thanks.

Hi Bruno,

Pls find the link to "horizontal sum" patch below.

http://reviews.llvm.org/D10964

Regards,
Shahid

hfinkel added inline comments.Jul 9 2015, 7:39 PM
lib/CodeGen/SelectionDAG/LegalizeDAG.cpp
2699 ↗(On Diff #28854)

Why are you adding this in this patch?

2935 ↗(On Diff #28854)

Both ISD::UABSDIFF and ISD::SABSDIFF are the same?

Hi Hal,

Thanks for the comments, response Inlined.

Regards,
Shahid

lib/CodeGen/SelectionDAG/LegalizeDAG.cpp
2699 ↗(On Diff #28854)

In absence of this, instruction selection was hitting "LLVM ERROR: Cannot select: vselect".

2935 ↗(On Diff #28854)

Correct me if I am wrong, I think with respect to expansion here both are same as the signedness of 'absolute diff' operation depends on the 1st 'SUB' operation.

hfinkel added inline comments.Jul 10 2015, 3:03 AM
lib/CodeGen/SelectionDAG/LegalizeDAG.cpp
2699 ↗(On Diff #28854)

If we're just plain missing code to expand VSELECT, I'd think you should be able to trigger that with IR for some target, and test it, directly. I'd prefer this be a separate commit if possible.

2935 ↗(On Diff #28854)

Ah, I see what you mean. The difference at the IR level is that SABSDIFF has NSW on the SUBs, and the unsigned version does not. The nice thing is that we can now do this at the SDAG level too..

SDNodeFlags Flags;
Flags.setNoSignedWrap(true);

Tmp1 = DAG.getNode(ISD::SUB, dl, VT, Tmp2, Tmp3, &Flags);

(and similar for the other nodes)

jmolloy added inline comments.Jul 10 2015, 3:32 AM
lib/CodeGen/SelectionDAG/LegalizeDAG.cpp
2935 ↗(On Diff #28854)

I think ISD::SETLT should also change to ISD::SETULT depending on signedness.

ashahid added inline comments.Jul 10 2015, 4:42 AM
lib/CodeGen/SelectionDAG/LegalizeDAG.cpp
2699 ↗(On Diff #28854)

ExpandVselect is present in LegalizeVecctorOps, so triggering just VSELECT works fine.However our case is in fact "Expansion of VSELECT during expansion of ABSDIFF" which is why this error is being reported.

2935 ↗(On Diff #28854)

Oh nice, so NSW and frnds are diving deep :)

ashahid edited edge metadata.

The updated revision has slight document change.Another change is to propagate NSW flag to appropriate SDAG.
It also has a change for intrinsic declaration.

The expansion of *ABSDIFF node has been moved to LegalizeVectorOps from LegalizeDAG as this is a vector operation.

jmolloy added inline comments.Jul 15 2015, 12:27 AM
lib/CodeGen/SelectionDAG/LegalizeVectorOps.cpp
725

This is unnecessarily complex. You've duplicated most of the code, whereas the only things that need changing are:

Flags.setNoSignedWrap(Op->getOpcode() == ISD::SABSDIFF);

and

DAG.getCondCode(Op->getOpcode() == ISD::SABSDIFF ? ISD::SETLT : ISD::SETULT);

Refactored *ABSDIFF* expansion code.

LGTM! Please wait for Hal's OK too.

hfinkel accepted this revision.Jul 15 2015, 10:49 PM
hfinkel edited edge metadata.

LGTM too.

This revision is now accepted and ready to land.Jul 15 2015, 10:49 PM

Hi Hal/James,

Since I don't have commit access yet, can somebody commit it on my behalf.

Regards,
Shahid

ashahid edited edge metadata.
ashahid removed rL LLVM as the repository for this revision.

Rebased and resolved the error.

jmolloy closed this revision.Jul 17 2015, 2:23 AM

This got committed.