This is an archive of the discontinued LLVM Phabricator instance.

[AMDGPU]: Fixes an invalid clamp selection pattern.
ClosedPublic

Authored by tsymalla on Feb 2 2021, 9:29 AM.

Details

Summary

When running the tests on PowerPC and x86, the lit test GlobalISel/trunc.ll fails at the memory sanitize step. This seems to be due to wrong invalid logic (which matches even if it shouldn't) and likely missing variable initialisation."

Diff Detail

Event Timeline

tsymalla created this revision.Feb 2 2021, 9:29 AM
tsymalla requested review of this revision.Feb 2 2021, 9:29 AM
Herald added a project: Restricted Project. · View Herald TranscriptFeb 2 2021, 9:29 AM
arsenm added inline comments.Feb 2 2021, 9:30 AM
llvm/lib/Target/AMDGPU/AMDGPUPreLegalizerCombiner.cpp
31–167

I don't see why you need to uglify this. Can't you just reset MatchInfo before the second matcher?

tsymalla added inline comments.Feb 2 2021, 9:41 AM
llvm/lib/Target/AMDGPU/AMDGPUPreLegalizerCombiner.cpp
31–167

The problem is: in the current implementation, if both of the matches fail, there is a fallthrough. If there is no min or max following the trunc (see the test case), the match will continue. I want to make sure at least one case exists. That's why I am double-checking here.

arsenm added inline comments.Feb 2 2021, 9:50 AM
llvm/lib/Target/AMDGPU/AMDGPUPreLegalizerCombiner.cpp
31–167

Can you extract this into a separate function and call it in the passing cases instead?

tsymalla updated this revision to Diff 321101.Feb 3 2021, 8:16 AM

[AMDGPU] Re-adding the changes from D93708 and fixing the clamp selection pattern.
This patch intends to re-add the changes from D93708 which were removed in d49efdc9696afee4b972c54bc3678b28c5700047 and should fix the issue adressed in this update.
This should fix the sanitizer builds and prevent applying the clamp in unwanted cases.

I extracted the corresponding parts into a separate lambda and improved the logic to only apply the combine in the corresponding cases where either a min-max or max-min pattern exists in the MIR.
As @Flakebi reverted the changes to prevent the sanitizer from failing, I re-added the changes to this revision.

tsymalla marked an inline comment as done.Feb 3 2021, 8:21 AM
arsenm accepted this revision.Feb 3 2021, 8:33 AM
This revision is now accepted and ready to land.Feb 3 2021, 8:33 AM
This revision was landed with ongoing or failed builds.Feb 8 2021, 4:06 AM
This revision was automatically updated to reflect the committed changes.