Page MenuHomePhabricator

Add support for casting elements in vectors for certain Std dialect type conversion operations.
ClosedPublic

Authored by llitchev on Wed, Sep 9, 12:39 PM.

Details

Summary

Added support to the Std dialect cast operations to do casts in vector types when feasible.

Diff Detail

Event Timeline

llitchev created this revision.Wed, Sep 9, 12:39 PM
Herald added a project: Restricted Project. · View Herald Transcript
llitchev requested review of this revision.Wed, Sep 9, 12:40 PM
bondhugula requested changes to this revision.Wed, Sep 9, 10:13 PM
bondhugula added inline comments.
mlir/lib/Dialect/StandardOps/IR/Ops.cpp
224

Doc comment please (triple /). Please describe what the method referring to its args.

2308–2313

All of these changes would be inconsistent with the current documentation / spec of these ops. See Ops.td file - those have to be updated first if the semantics indeed changed. For eg. UIToFP's doc comment says:

let description = [{
    Cast from a value interpreted as unsigned integer to the corresponding
    floating-point value. If the value cannot be exactly represented, it is
    rounded using the default rounding mode. Only scalars are currently
    supported.
  }];

We can't change the behavior while having the spec/doc to the contrary.

2390

Comment would be outdated.

This revision now requires changes to proceed.Wed, Sep 9, 10:13 PM
llitchev updated this revision to Diff 291061.Thu, Sep 10, 12:24 PM
llitchev marked 2 inline comments as done.

Resolved CR feedback - updated docs.

mlir/lib/Dialect/StandardOps/IR/Ops.cpp
224

Fixed.

2308–2313

Fixed.

2390

Fixed.

bondhugula requested changes to this revision.Thu, Sep 10, 5:33 PM

I find the commit summary and the title a bit confusing. Isn't this extending standard dialect cast ops to work with vectors of those types? It's not that vector types are being cast to int/fp.

There are operations that support casting for vector types, but the code was missing. The necessary code and tests were added.

Please rephrase. Those operations existed in the LLVM dialect not in std dialect.

The test cases for a key change to a std dialect op's semantics should reside in the std dialect's test cases, and not just in the LLVM dialect ones.

mlir/include/mlir/Dialect/StandardOps/IR/Ops.td
2446–2450

Nit: Reflow. Please use the whole width.

3129–3131

Likewise.

This revision now requires changes to proceed.Thu, Sep 10, 5:33 PM
bondhugula added inline comments.Thu, Sep 10, 5:35 PM
mlir/lib/Dialect/StandardOps/IR/Ops.cpp
221

Nit: period at the end.

225

Sentence broken.

226–228

Please rephrase - this isn't really well-formed.

llitchev retitled this revision from Add support for casting of vector types to certain FP/Integer operations to Add support for casting elements in vectors for certain Std dialect type conversion operations..Fri, Sep 11, 11:15 AM
llitchev edited the summary of this revision. (Show Details)
llitchev marked 5 inline comments as done.Fri, Sep 11, 11:19 AM

Rewritten the title and the summary as well.
The tests are in the StandardToLLVM directory (as the rest of the similar conversion tests). This is not part of the LLVM? Or maybe I am wrong?

mlir/include/mlir/Dialect/StandardOps/IR/Ops.td
2446–2450

Fixed. Using no more than 80 characters.

3129–3131

Fixed. Using no more than 80 characters.

mlir/lib/Dialect/StandardOps/IR/Ops.cpp
221

Fixed.

225

Rewritten.

226–228

Rewritten.

llitchev updated this revision to Diff 291288.Fri, Sep 11, 11:23 AM
llitchev marked 5 inline comments as done.

Addressed some CR feedback.

ftynse accepted this revision.Mon, Sep 14, 12:27 AM
This revision is now accepted and ready to land.Mon, Sep 14, 7:45 AM