This is an archive of the discontinued LLVM Phabricator instance.

[GlobalISel] Use more MachineIRBuilder helper methods
ClosedPublic

Authored by foad on Jan 16 2020, 4:17 AM.

Diff Detail

Event Timeline

foad created this revision.Jan 16 2020, 4:17 AM

Unit tests: pass. 61909 tests passed, 0 failed and 783 were skipped.

clang-tidy: unknown.

clang-format: pass.

Build artifacts: diff.json, clang-format.patch, CMakeCache.txt, console-log.txt, test-results.xml

arsenm accepted this revision.Jan 16 2020, 5:24 AM
arsenm added inline comments.
llvm/lib/CodeGen/GlobalISel/LegalizerHelper.cpp
1441

This should be fixed separately l, but the getZExtValue is unnecessary and will break for wider types

This revision is now accepted and ready to land.Jan 16 2020, 5:24 AM
foad updated this revision to Diff 238468.Jan 16 2020, 5:39 AM

Added similar fixes in IRTranslator.cpp.

foad marked an inline comment as done.Jan 16 2020, 5:45 AM

Sorry @arsenm, I updated the patch just before I saw your approval.

llvm/lib/CodeGen/GlobalISel/LegalizerHelper.cpp
1997

In case you were wondering about this hunk: the old code left the G_SMULO/UMULO in place, which defined Res, and also created a new G_MUL, which defined Res. This caused an assertion failure when buildAShr tried to constant fold its arguments, because it wasn't expecting to find more than one definition of Res. Is this a general problem in GlobalISel lowering and is my workaround reasonable?

Unit tests: pass. 61909 tests passed, 0 failed and 783 were skipped.

clang-tidy: unknown.

clang-format: pass.

Build artifacts: diff.json, clang-format.patch, CMakeCache.txt, console-log.txt, test-results.xml

arsenm added inline comments.Jan 16 2020, 6:31 AM
llvm/lib/CodeGen/GlobalISel/LegalizerHelper.cpp
1991

MIRBuilder.getII()

1997

It looks to me like the old code didn't modify the instruction in place, the new code does. Most places create new defs of the original result before erasing the original instruction, so this might be a widespread issue with the CSEMIRBuilder

arsenm added inline comments.
llvm/lib/CodeGen/GlobalISel/LegalizerHelper.cpp
1997

Can you split this part into a separate change?

foad updated this revision to Diff 238484.Jan 16 2020, 6:52 AM

Remove G_SMULO/G_UMULO changes which have been split out into D72842.

foad marked an inline comment as done.Jan 16 2020, 6:54 AM

Unit tests: pass. 61909 tests passed, 0 failed and 783 were skipped.

clang-tidy: unknown.

clang-format: pass.

Build artifacts: diff.json, clang-format.patch, CMakeCache.txt, console-log.txt, test-results.xml

arsenm accepted this revision.Jan 16 2020, 7:15 AM

LGTM

This revision was automatically updated to reflect the committed changes.
foad marked 2 inline comments as done.Jan 16 2020, 7:58 AM
foad added inline comments.
llvm/lib/CodeGen/GlobalISel/LegalizerHelper.cpp
1441

See D72853.