Page MenuHomePhabricator

[AArch64ISelDAGToDAG] Supplement cases for ORRWrs/ORRXrs when calculating usefulbits
ClosedPublic

Authored by TiehuZhang on May 19 2021, 4:00 AM.

Details

Summary

For the case,

    
t8: i32 = or t7, t4
t10: i32 = ORRWrs t8, t8, TargetConstant:i32<73>

the userfulbits of t8 should be full bits of itself (t8 | (t8 >> shiftConstant)). Current implementation doesn't catch the case and may get a wrong result. The patch supplements the missing case.

Diff Detail

Event Timeline

TiehuZhang created this revision.May 19 2021, 4:00 AM
TiehuZhang requested review of this revision.May 19 2021, 4:00 AM
Herald added a project: Restricted Project. · View Herald TranscriptMay 19 2021, 4:00 AM
TiehuZhang edited the summary of this revision. (Show Details)May 19 2021, 4:01 AM
mdchen added inline comments.May 19 2021, 4:23 AM
llvm/test/CodeGen/AArch64/arm64-isel-or.ll
14

This function is irrelevant to the issue and could be removed I think.

59

The test case is not supposed to do output. Please remove the printf here; also the consequently unused declaration and other related elements.

update the testcase

TiehuZhang marked 2 inline comments as done.May 23 2021, 6:21 PM

ping

llvm/test/CodeGen/AArch64/arm64-isel-or.ll
59

Done, thanks.

LGTM, but please wait for a couple of days for experts in this area to review: )

sdesmalen added inline comments.Jun 2 2021, 7:35 AM
llvm/lib/Target/AArch64/AArch64ISelDAGToDAG.cpp
2309

nit: Can you write it as:

case AArch64::ORRWrs:
case AArch64::ORRXrs:
  if (UserNode->getOperand(0) != Orig && UserNode->getOperand(1) == Orig)
    getUsefulBitsFromOrWith..(...);
  return;

That's a little easier to read.

llvm/test/CodeGen/AArch64/arm64-isel-or.ll
2

Instead of stopping after instruction selection, I think it's easier to just match the output asm directly, since you can now check that some of the instructions are no longer removed (because they were previously undef). If you use the update_llc_test_checks.py script, you can match the output directly.

13

You can simplify the IR by taking the value as input argument instead of loading it, and returning it instead of storing it.

Also dso_local and local_unnamed_addr can be removed.

16

Can you shorten the test-file by using a larger shift value (and less shift/ors)?

18

Can you also add a similar test-case for shl?

efriedma added inline comments.
llvm/lib/Target/AArch64/AArch64ISelDAGToDAG.cpp
2320

Do these store instructions have the same issue?

TiehuZhang updated this revision to Diff 349545.Jun 3 2021, 7:32 AM
TiehuZhang marked an inline comment as done.
TiehuZhang marked 5 inline comments as done.Jun 6 2021, 6:15 PM

ping

llvm/lib/Target/AArch64/AArch64ISelDAGToDAG.cpp
2309

Done, thanks for your suggestion

2320

If x(an usee) is used on an unpredicatable instruction, then all its bits are useful. The current implementation does not seem to be exceptional, I think.

TiehuZhang marked an inline comment as done.Mon, Jun 28, 10:38 PM

ping

sdesmalen accepted this revision.Tue, Jun 29, 1:25 AM

LGTM, thanks!

This revision is now accepted and ready to land.Tue, Jun 29, 1:25 AM