Page MenuHomePhabricator

[SelectionDAG] Fix return calling convention in expansion of ?MULO
ClosedPublic

Authored by whitequark on Apr 6 2018, 12:24 AM.

Details

Summary

The SMULO/UMULO DAG nodes, when not directly supported by the target,
expand to a multiplication twice as wide. In case that the resulting
type is not legal, the legalizer cannot directly call the intrinsic with
the wide arguments; instead, it "pre-lowers" them by splitting them
in halves.

rL283203 made sure that on big endian targets, the legalizer passes
the argument halves in the correct order. It did not do the same
for the return value halves because the existing code used a hack;
it put an illegal type into DAG and hoped that nothing would break
and it would be correctly lowered elsewhere.

rL307207 fixed this, handling return value halves similar to how
argument handles are handled, but did not take big-endian targets
into account.

This commit fixes the expansion on big-endian targets, such as
the out-of-tree OR1K target.

Diff Detail

Repository
rL LLVM

Event Timeline

whitequark created this revision.Apr 6 2018, 12:24 AM
whitequark updated this revision to Diff 141296.Apr 6 2018, 2:17 AM

Fix a typo.

Can you make a testcase for some in-tree target affected by this bug? From what I understand, thumbebv6-linux-gnueabi should be affected by this issue.

lib/CodeGen/SelectionDAG/LegalizeDAG.cpp
3622 ↗(On Diff #141296)

Whitespace.

@whitequark Thank you for the patch. Are you still interested in pursuing it?
I attached a testcase to prove that the registers are swapped on big-endian systems.

@george-hopkins Thanks, I will take a look.

Added a test, based on the one provided by @george-hopkins but modified to pass on master.

Herald added a project: Restricted Project. · View Herald TranscriptFeb 11 2019, 9:13 AM
RKSimon added subscribers: nikic, RKSimon.

Adding @nikic as there's crossover with D58006

Please can add diff context?

svn diff --diff-cmd=diff -x -U999999
https://llvm.org/docs/Phabricator.html#requesting-a-review-via-the-web-interface

Added diff context

efriedma added inline comments.Feb 11 2019, 1:05 PM
lib/CodeGen/SelectionDAG/LegalizeDAG.cpp
3439 ↗(On Diff #186277)

Whitespace

test/CodeGen/Thumb/umulo-32-legalisation-lowering.ll
36 ↗(On Diff #186277)

I think you could simplify this to just return the i1, without losing any generality. If you do that, it's also much less likely that register allocation changes will affect the CHECK lines in the future.

Addressed review

This revision is now accepted and ready to land.Feb 12 2019, 8:38 AM
This revision was automatically updated to reflect the committed changes.