This is an archive of the discontinued LLVM Phabricator instance.

[mlir][ArmSME] Fix get_tile_id type in zero lowering
ClosedPublic

Authored by c-rhodes on Aug 29 2023, 3:37 AM.

Details

Summary

The arm_sme.get_tile_id op returns a scalar integer but the arm_sme.zero
op lowering incorrectly uses the element type, which could be
floating-point.

Diff Detail

Event Timeline

c-rhodes created this revision.Aug 29 2023, 3:37 AM
Herald added a reviewer: dcaballe. · View Herald Transcript
Herald added a project: Restricted Project. · View Herald Transcript
c-rhodes requested review of this revision.Aug 29 2023, 3:37 AM

I think this op is a bit clunky and could be simplified by returning the element type of the tile rather than an int representing the bitwidth, or perhaps just the 2-D scalable vector type this is extracted from, but that's a slightly move involved refactor.

I think this op is a bit clunky and could be simplified by returning the element type of the tile rather than an int representing the bitwidth, or perhaps just the 2-D scalable vector type this is extracted from, but that's a slightly move involved refactor.

Which Op are you referring to arm_sme.zero? arm_sme.cast_tile_to_vector?

mlir/test/Dialect/ArmSME/tile-zero-masks.mlir
6

How is "8-bit" relevant here? What's meant to be 8-bit?

9

[nit] DELETME

I think this op is a bit clunky and could be simplified by returning the element type of the tile rather than an int representing the bitwidth, or perhaps just the 2-D scalable vector type this is extracted from, but that's a slightly move involved refactor.

Which Op are you referring to arm_sme.zero? arm_sme.cast_tile_to_vector?

arm_sme.get_tile_id

mlir/test/Dialect/ArmSME/tile-zero-masks.mlir
6

How is "8-bit" relevant here? What's meant to be 8-bit?

the zero instruction mask, see https://armv8.arm.com/latest_builds/v9A/isa64/zero_za_i.xml

c-rhodes added inline comments.Aug 29 2023, 5:03 AM
mlir/test/Dialect/ArmSME/tile-zero-masks.mlir
6

How is "8-bit" relevant here? What's meant to be 8-bit?

the zero instruction mask, see https://armv8.arm.com/latest_builds/v9A/isa64/zero_za_i.xml

Apologies that's an internal link, external link: https://developer.arm.com/documentation/ddi0602/2022-06/SME-Instructions/ZERO--Zero-a-list-of-64-bit-element-ZA-tiles-

benmxwl-arm accepted this revision.Aug 29 2023, 10:14 AM

My bad, LGTM!

This revision is now accepted and ready to land.Aug 29 2023, 10:14 AM
awarzynski accepted this revision.Aug 29 2023, 11:05 AM

LGTM, thanks! My comments are nits, so feel free to ignore.

mlir/test/Dialect/ArmSME/tile-zero-masks.mlir
6

Thanks, now I see what you meant!

It's a bit confusing that this is referring to "8-bit" tile masks, but there are no 8-bit tile masks here :) Perhaps:

// This test verifies the tile mask operand of the zero intrinsic zeroes
// the correct tiles. Both integer and floating-point datatypes are checked.
// Note that once lowered to ASM/machine code, these masks are narrowed to 8 bit:
//   * https://developer.arm.com/documentation/ddi0602/2022-06/SME-Instructions/ZERO--Zero-a-list-of-64-bit-element-ZA-tiles-

WDYT? Did I get it right?

This revision was automatically updated to reflect the committed changes.
c-rhodes marked an inline comment as done.Aug 30 2023, 12:20 AM
c-rhodes added inline comments.
mlir/test/Dialect/ArmSME/tile-zero-masks.mlir
6

I've just removed mention of 8-bit mask altogether