This is an archive of the discontinued LLVM Phabricator instance.

[mlir][StandardToSPIRV] Handle i1 case for lowering memref.load/store op
ClosedPublic

Authored by hanchung on Apr 1 2021, 6:47 AM.

Details

Summary

This patch unconditionally converts i1 types to i8 types on memrefs. If the
extensions or capabilities are not met, they will be converted to i32. Hence the
logic in IntLoadPattern and IntStorePattern are also updated.

Also added the implementation of SPIRVTypeConverter::getOptions().

Diff Detail

Event Timeline

hanchung created this revision.Apr 1 2021, 6:47 AM
hanchung requested review of this revision.Apr 1 2021, 6:47 AM
hanchung updated this revision to Diff 334709.Apr 1 2021, 8:34 AM

fix debug build

hanchung updated this revision to Diff 334711.Apr 1 2021, 8:38 AM

use build-in function

I hit a really weird thing... and I can only repro with this PR...

module attributes {
  spv.target_env = #spv.target_env<
    #spv.vce<v1.0, [Int16, StorageBuffer16BitAccess, Shader],
    [SPV_KHR_storage_buffer_storage_class, SPV_KHR_16bit_storage]>, {}>
} {

func @load_i8(%arg0: memref<i8>) -> i8 {
  %0 = memref.load %arg0[] : memref<i8>
  return %0 : i8
}

}

The above one can be lowered to SPIR-V, but the below one can not

module attributes {
  spv.target_env = #spv.target_env<
    #spv.vce<v1.0, [Int16, StorageBuffer16BitAccess, Shader],
    [SPV_KHR_storage_buffer_storage_class, SPV_KHR_16bit_storage]>, {}>
} {

func @load_i1(%arg0: memref<i1>) -> i1 {
  %0 = memref.load %arg0[] : memref<i1>
  return %0 : i1
}

}

They are expected to generate the same IRs. And they do when there are no users.

The i1 case only works if it does not have users, like in the test.

I dumped operands[0] in ReturnOpPattern. i8 case showed the replaced op %15 = spv.ShiftRightArithmetic %14, %13 : i32, i32, and i1 case showed the old one %16 = memref.load <<UNKNOWN SSA VALUE>>[] : memref<i1>. It is weird because it said that the pattern applied successfully, but the users still get the old operand.

I also verified that i1 case and i8 case generate the same log with -debug and they are all marked legal and success.

I'd be appreciate if someone can help on this... It looks like a memref.load was created because the variable name is %16.

To repro, run mlir-opt -convert-std-to-spirv a.mlir

full log for i1: https://gist.github.com/hanhanW/8a4494f337629299959b6e7d59a882f5
full log for i8: https://gist.github.com/hanhanW/56a8fcdd6a8eb25a743a71667754e6e4

hanchung planned changes to this revision.Apr 6 2021, 9:54 AM

@antiagainst is going to add some support to SPIR-V, and I will rebase on it and rework a bit.

I've created D100058 and D100059 to introduce controls over emulation. You can rebase after they are landed. With D100059 I think we can introduce another option there to control the # of bits for boolean types. So the getTypeNumBytes (which should probably be changed to getTypeNumBits) and all those convert*Type functions can properly handle i1.

hanchung updated this revision to Diff 336105.Apr 8 2021, 7:35 AM

fix i1 issues

Fixed, and enhanced the test that loads i1. (now it will return i1 to make sure there is no such issue again.)

antiagainst requested changes to this revision.Apr 8 2021, 9:02 AM

Mostly okay to me. Major request to have an option for control.

mlir/lib/Conversion/StandardToSPIRV/StandardToSPIRV.cpp
995

s/isInt1/isBool/

1052

W already have typeConverter at L988.

mlir/lib/Dialect/SPIRV/Transforms/SPIRVConversion.cpp
344

s/convertMemrefI1Type/convertBoolMemrefType/

349

s/yte/yet/

377

Can you add an option in SPIRVTypeConverter::Options, like boolNumBits, to control this? (For now we can just support the case == 8 and bail out on type conversion for other cases.)

This revision now requires changes to proceed.Apr 8 2021, 9:02 AM
hanchung updated this revision to Diff 336167.Apr 8 2021, 10:42 AM
hanchung marked 5 inline comments as done.

address comments

mlir/lib/Conversion/StandardToSPIRV/StandardToSPIRV.cpp
1052

I did not notice it, thanks!

mlir/lib/Dialect/SPIRV/Transforms/SPIRVConversion.cpp
377

good point, it is useful

Also added the implementation of SPIRVTypeConverter::getOptions().

hanchung edited the summary of this revision. (Show Details)Apr 8 2021, 10:43 AM
antiagainst accepted this revision.Apr 8 2021, 11:01 AM

Cool, just one final comment.

mlir/lib/Dialect/SPIRV/Transforms/SPIRVConversion.cpp
156

Oops, sorry I forgot about this. :) Thanks for fixing it!

365

I think here it should be

c++
if (numBoolBits != 8) {
  LLVM_DEBUG(llvm::dbgs() << "using non-8-bit storage for bool types unimplemented");
  return nullptr;
}

?

This revision is now accepted and ready to land.Apr 8 2021, 11:01 AM
hanchung updated this revision to Diff 336179.Apr 8 2021, 11:45 AM
hanchung marked 2 inline comments as done.

address comments

mlir/lib/Dialect/SPIRV/Transforms/SPIRVConversion.cpp
365

good point! I feel that it will work, but did not test it. Let's have the check, and we can turn it back and add tests when we need it.

This revision was landed with ongoing or failed builds.Apr 8 2021, 12:15 PM
This revision was automatically updated to reflect the committed changes.