This is an archive of the discontinued LLVM Phabricator instance.

[mlir][llvm] Tighten LLVM dialect intrinsic type constraints.
ClosedPublic

Authored by gysit on Oct 20 2022, 7:46 AM.

Details

Summary

The revision specifies more precise argument and result type
constraints for many of the llvm intrinsics. Additionally, add
tests to verify intrinsics with invalid arguments/result result
in a verification error.

Diff Detail

Event Timeline

gysit created this revision.Oct 20 2022, 7:46 AM
Herald added a project: Restricted Project. · View Herald Transcript
gysit requested review of this revision.Oct 20 2022, 7:46 AM
ftynse accepted this revision.Oct 25 2022, 2:06 AM
ftynse added inline comments.
mlir/include/mlir/Dialect/LLVMIR/LLVMIntrinsicOps.td
22

I suspect we want AnySignlessInteger here and below, MLIR integers can have signs and we don't want those in the LLVM intrinsics.

mlir/test/Target/LLVMIR/llvmir-invalid.mlir
138

Nit: FileCheck now supports @below instead of @+1 that reads better and works when combining errors and notes.

This revision is now accepted and ready to land.Oct 25 2022, 2:06 AM
gysit updated this revision to Diff 470450.Oct 25 2022, 5:44 AM

Address comments:

  • use @below instead of @+1
  • use AnySignlessInteger for all intrinsics

Rebase (after https://reviews.llvm.org/D136498):

  • use qualified(type($ptr)) for arguments of type LLVM_AnyPointer
  • adapt expected error messages
gysit marked 2 inline comments as done.Oct 25 2022, 5:50 AM

@dcaballe I changed the integer type constraints on the vector predicate intrinsics from AnyInteger to AnySignlessInteger as well. I hope this does not conflict with your changes.

@dcaballe I changed the integer type constraints on the vector predicate intrinsics from AnyInteger to AnySignlessInteger as well. I hope this does not conflict with your changes.

That should be fine! Thanks for pinging me :)

mlir/include/mlir/Dialect/LLVMIR/LLVMIntrinsicOps.td
352

Could you comment on why we need this vs just taking a fast math flags attr ?
This seems quite inconsistent with other places.

gysit added inline comments.Sep 20 2023, 5:49 AM
mlir/include/mlir/Dialect/LLVMIR/LLVMIntrinsicOps.td
352

As far as I remember the reason for this was that vector reduction instructions only support allowReassoc (they do not have a fast math flag attribute attached). I would assume that is a limitation of the op itself and I didn't want to change the op semantics at the time. If LLVM IR actually supports the full fastmath attribute it could be added to the LLVM dialect operation as well.

gysit added inline comments.Sep 20 2023, 5:53 AM
mlir/include/mlir/Dialect/LLVMIR/LLVMIntrinsicOps.td
352

It seems like the reassoc is the only thing that is supported on these intrinsics:

%unord = call reassoc float @llvm.vector.reduce.fmul.v4f32(float 1.0, <4 x float> %input) ; relaxed reduction

At least this is mentioned specifically in the language ref https://llvm.org/docs/LangRef.html#id826