This is an archive of the discontinued LLVM Phabricator instance.

Add intrinsics and SDNodes for signed/unsigned absolute difference (absdiff) and horizontal add (hadd).
AbandonedPublic

Authored by rengolin on Jun 5 2015, 12:45 AM.

Details

Summary

This adds new intrinsics "*absdiff" and "*hadd" to facilitate the efficient code generation for "sum of absolute differences" operation.

The patch also contains the introduction of corresponding SDNodes and basic legalization support

An RFC discussion could be found at below link
https://groups.google.com/forum/#!topic/llvm-dev/S3P1Sh3H--Q

Diff Detail

Repository
rL LLVM

Event Timeline

ashahid retitled this revision from to Add intrinsics and SDNodes for signed/unsigned absolute difference (absdiff) and horizontal add (hadd)..
ashahid updated this object.
ashahid edited the test plan for this revision. (Show Details)
ashahid added reviewers: hfinkel, jmolloy, rengolin.
ashahid set the repository for this revision to rL LLVM.
ashahid added a subscriber: Unknown Object (MLST).
jmolloy requested changes to this revision.Jun 8 2015, 1:03 AM
jmolloy edited edge metadata.

Hi,

Thanks for working on this!

I have a bunch of comments, and this is just a first round review. Also, this needs to be split into three separate patches:

  1. Addition of sabsdiff/uabsdiff
  2. Addition of hadd
  3. X86 changes

Each should have testcases. The first two don't currently; you're only testing the X86 backend's handling of the intrinsics which may be fine, but you do need to ensure you're checking the generic EXPAND behaviour as well as the specific X86 behaviour you've added in patch (3).

Cheers,

James

docs/LangRef.rst
9562

I don't think floating point datatypes should be bundled in with integer datatypes in these intrinsics. Signedness isn't a concept for floating point datatypes, so it would be weird to have something operate on floats with a defined signedness. Also, it would mean two intrinsics were identical for floating point types which raises a canonicalisation problem.

I'd be happy if the floating point support for these intrinsics was removed for the time being, if you don't want to add another intrinsic right now.

9586

What does this note mean, in terms of action required by the reader? Do you mean that users should aim not to write these intrinsics? If so, what should they do instead?

9619

Because addition in wrapping 2's complement arithmetic is sign independent, you shouldn't need a signed and unsigned version of this. Just "llvm.hadd" should do.

include/llvm/CodeGen/ISDOpcodes.h
335

I'd prefer "SABSDIFF" here to match the intrinsic better and be more self documenting.

include/llvm/Target/TargetSelectionDAG.td
388

I'd prefer "sabsdiff" here to match the intrinsic name better, and be more self documenting.

lib/CodeGen/SelectionDAG/LegalizeDAG.cpp
2888

What happens if the "if" condition fails? Tmp1 is a dangling pointer at the moment.

2893

Please write how you expect the expansion to be improved.

lib/CodeGen/SelectionDAG/LegalizeIntegerTypes.cpp
160

I'm not sure about this - I think you shouldn't need to care about the high bits of a promoted value (that's the responsibility of PromoteIntOp of the user), so you can just use PromoteIntRes_SimpleBinOp here instead.

908

Why are you not handling ISD::SABD and friends here?

1243

You don't need this. Because addition is agnostic to the high bits, you don't need to extend anything. You can just use PromoteIntOp_SimpleIntBinOp here.

lib/CodeGen/SelectionDAG/SelectionDAGDumper.cpp
229

Indentation error.

This revision now requires changes to proceed.Jun 8 2015, 1:03 AM

Hi James,

Thanks for your comments.

I am on vacation now. I will look into it accordingly afterwards.

Regards,
Shahid

ashahid added inline comments.Jun 30 2015, 2:30 AM
docs/LangRef.rst
9562

Thanks, I would prefer to defer floating point intrinsics for time being.

9586

In fact I meant this.
"These intrinsics are primarily used during code generation stage of compilation.They are generated by the compiler passes such as Loop/SLP vectorizer."

9619

Even I was not sure about this. However, as ARM has both signed / unsigned horizontal add instruction, how would you decide to to emit the proper one?

include/llvm/CodeGen/ISDOpcodes.h
335

Will handle it in follow up patches.

include/llvm/Target/TargetSelectionDAG.td
388

Will handle it in follow up patches.

lib/CodeGen/SelectionDAG/LegalizeDAG.cpp
2888

Will be handled in follow up patches.

2893

Oops, comment slipped

lib/CodeGen/SelectionDAG/LegalizeIntegerTypes.cpp
160

Will be handled in follow up patches.

908

Will be handled accordingly.

jmolloy resigned from this revision.Jul 17 2015, 2:24 AM
jmolloy removed a reviewer: jmolloy.

[Phab spring clean] This patch has been usurped by D10964 and D10967.

rengolin edited edge metadata.Jul 18 2015, 6:53 AM

Shall we abandon it, then?

@ashahid?

Hi,

Yes, this can be abandoned.

Sorry for the delayed response as I am not keeping well these days.

Regards,
Shahid

rengolin commandeered this revision.Jul 28 2015, 4:13 AM
rengolin abandoned this revision.
rengolin edited reviewers, added: ashahid; removed: rengolin.