Add grouped binary convolution support (1/3): the converter. #549
Conversation
| if (total_input_channels % filter_input_channels != 0) { | ||
| mlir::emitError(filter_val.getLoc()) | ||
| << "Filter dimensions invalid: the number of filter input channels " | ||
| << filter_input_channels | ||
| << " does not divide the total number of input channels " | ||
| << total_input_channels << "\n"; | ||
| num_groups = 1; | ||
| } |
There was a problem hiding this comment.
Technically I don't think this is required, because the Conv2D op throws an error if this condition is violated, but it's probably better safe than sorry.
| if (num_groups > 1 && filter_input_channels % bitpacking_bitwidth != 0) { | ||
| mlir::emitError(filter_val.getLoc()) | ||
| << "Invalid binary group convolution: the number of input channels " | ||
| "per-group " | ||
| "must be a multiple of " | ||
| << bitpacking_bitwidth << ", but is " << filter_input_channels << "\n"; | ||
| num_groups = 1; | ||
| } |
There was a problem hiding this comment.
We could choose not to throw an error here, and simply not apply the transformation if this condition is violated, so that there will be a float op in the model. I'm not sure what the right approach is.
|
Upon reflection and discussions with @Tombana, there's actually technically no need to have a I'm not sure what the right thing to do here is. Conciseness is nice, but I worry that relying on being able to infer the number of groups by relying on that constraint being satisfied might lead to subtle bugs in the future. |
|
Even if we can infer it in theory, I think it's good to have it as a separate parameter, even if only as an extra correctness checksum. |
I'd actually prefer not to have a dedicated I think we can do all the necessary validation in In both cases we should always make sure that the number of filters per group is divisible by 32 in the converter anyway. |
|
I think I'm onboard with that. In case somebody tries to convert a grouped Larq bconv with non-multiple-of-32 group size, do you think we should throw a converter error (as this PR currently does) or fall back to an emulated bsign + float conv? |
I think it's fair to throw an error for now since TFLite doesn't support group convolutions anyway so it would fail during runtime anyway. |
AdamHillier
force-pushed
the
grouped-convolutions-converter
branch
from
9c0acf3
to
1392f3e
Compare
Add a validation check to the converter to ensure that grouped convolutions have a group size that is a multiple of 32, and report an error otherwise.
AdamHillier
force-pushed
the
grouped-convolutions-converter
branch
from
1392f3e
to
1ba74aa
Compare
|
@lgeiger I've updated the PR so that it only performs the check, rather than adding the groups attribute. |
There was a problem hiding this comment.
This is great!
Do you think it would make sense to move this check into an op verifier for the bconv op as mentioned in #406 instead? This way the check would be op specific instead of limited to this transformation. Although I am not sure if we still have all of the information there since the bconv op will already have bitpacked inputs and migth either have float or binary filters.
I really like this idea in theory, but you're right that it won't be possible once the filters have been bitpacked. E.g. the number of input channels could be 64, and the bitpacked filter channels in dimension could be 1, and we don't know if that's because it's 32 input channels to the filter (which is valid) or only 1 that's been padded by bitpacking (1 input channel as in a depthwise conv, which would not be valid). |
That's true. In theory I think it woud still work since the filters will be bitpacked in a later pass, but this means that the verifier would be tied to this transformation anyway. Let's keep it like this for now. |
What do these changes do?
This is first of a group of PRs to add support for grouped binary convolutions. I've split the work into three PRs to make review easier.
First, this PR adds support to the converter, by adding an attribute
groupsto the op definition. The correct value forgroupsis infered by dividing the input channels by thechannels_indimension of the filter. An error is raised if the size of each group (the input channels divided bygroups) is not a multiple of 32 -- I have chosen to add this constraint because it significantly simplifies the implementation in the kernels.How Has This Been Tested?
MLIR file-check tests have been added to check that the attribute is correctly set, and that the error is raised if the group-size is not a multiple of 32.
Benchmark Results
N/A.
Related issue number
#550, #551.