TorchToTosa: ConvertAtenAvgPool2dOp is too restrictive.

[dbabokin]

PyTorch AvgPool2d has count_include_pad parameter, while TOSA's AvgPool2d doesn't have such parameter.

With recent fix 7f9f99c by @Hanumanth04 the lowering of AvgPool2d was disabled, if count_include_pad is True to avoid silent wrong answer. The problem is that the fix doesn't take into account the padding value. If the padding is zero, then the value count_include_pad doesn't matter.

Why is this important? Because PyTorch AvgPool2d by default have padding=0 and conut_include_pad=True:

torch.nn.AvgPool2d(kernel_size, stride=None, padding=0, ceil_mode=False, count_include_pad=True, divisor_override=None)

I.e. this breaks the default case.

[Hanumanth04]

Thanks for reporting this, @dbabokin; I appreciate it. I will take a look and make the change.

[sjarus]

Thanks for reporting this. I was just chatting with @justin-ngo-arm about the topic and we agree - anything that results in a regression of previously working tests would need to be re-investigated. At least the legalization should enable default support of prior working tests, or if the tests were previously insufficient, offer patches. Without this regression we should be able to achieve over 1000 passing TorchToTosa fx_importer_tosa e2e tests, which is a nice milestone in itself .

[justin-ngo-arm]

Hi @Hanumanth04, @sjarus and I also encounter the same problem. We were discussing looking at the e2e tests themselves and changing the parameters, but the problem might be more complicated than that since this also involves decomposing complex ops and PyTorch AvgPool2d have the default parameters like @dbabokin mentioned. I was going to raise an issue for discussion but @dbabokin has beaten me to it. Thank you for keeping an eye on this, @dbabokin!

By the way, the list of tests that got affected by this change is documented under the comment # count_include_pad and divisor_override check in TOSA AvgPool2d in FX_IMPORTER_TOSA_XFAIL_SET in xfail_sets.py. I added them there after @Hanumanth04's commit got merged. I might miss some other tests that got affected, but those are the ones that showed up when I ran the test suite target after the commit.

@Hanumanth04, if you find a change for AvgPool2d, can you please apply that to the AvgPool1d legalization I made last week too (#3855), or please let me know so I can make the same change to AvgPool1d which also has the same parameter and is legalized to TOSA AvgPool2d. Thank you!

[sahas3]

anything that results in a regression of previously working tests

@sjarus, @justin-ngo-arm is there any plan to run the fx_importer_tosa tests in the CI itself to hopefully catch these regressions early?

[Hanumanth04]

Hi @Hanumanth04, @sjarus and I also encounter the same problem. We were discussing looking at the e2e tests themselves and changing the parameters, but the problem might be more complicated than that since this also involves decomposing complex ops and PyTorch AvgPool2d have the default parameters like @dbabokin mentioned. I was going to raise an issue for discussion but @dbabokin has beaten me to it. Thank you for keeping an eye on this, @dbabokin!

By the way, the list of tests that got affected by this change is documented under the comment # count_include_pad and divisor_override check in TOSA AvgPool2d in FX_IMPORTER_TOSA_XFAIL_SET in xfail_sets.py. I added them there after @Hanumanth04's commit got merged. I might miss some other tests that got affected, but those are the ones that showed up when I ran the test suite target after the commit.

@Hanumanth04, if you find a change for AvgPool2d, can you please apply that to the AvgPool1d legalization I made last week too (#3855), or please let me know so I can make the same change to AvgPool1d which also has the same parameter and is legalized to TOSA AvgPool2d. Thank you!

@justin-ngo-arm, Sure, I can apply the fix for AvgPool1d as well. Thanks for documenting the failing tests. I will take a look to see if there are any other tests affected by this change.

[sjarus]

anything that results in a regression of previously working tests

@sjarus, @justin-ngo-arm is there any plan to run the fx_importer_tosa tests in the CI itself to hopefully catch these regressions early?

Yes we'd like to make fx_importer_tosa the default . It's currently pending on some dependencies from @mgehre-amd's flows. It would be preferable to run fx_importer_tosa rather than the tosa target (TorchScript import).