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

[TiehuZhang]

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.

[mdchen]

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.

[TiehuZhang]

update the testcase

[TiehuZhang]

ping

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

59

Done, thanks.

[mdchen]

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

[sdesmalen]

llvm/lib/Target/AArch64/AArch64ISelDAGToDAG.cpp

2308

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

1

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.

12

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.

15

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

17

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

[efriedma]

llvm/lib/Target/AArch64/AArch64ISelDAGToDAG.cpp

2319

Do these store instructions have the same issue?

[TiehuZhang]

ping

llvm/lib/Target/AArch64/AArch64ISelDAGToDAG.cpp

2308

Done, thanks for your suggestion

2319

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]

ping

[sdesmalen]

LGTM, thanks!

llvm/lib/Target/AArch64/AArch64ISelDAGToDAG.cpp

static void getUsefulBitsForUse(SDNode *UserNode, APInt &UsefulBits,
   case AArch64::UBFMWri:
   case AArch64::UBFMXri:
     return getUsefulBitsFromUBFM(SDValue(UserNode, 0), UsefulBits, Depth);
 
   case AArch64::ORRWrs:
   case AArch64::ORRXrs:
     if (UserNode->getOperand(1) != Orig)
       return;
+    if (UserNode->getOperand(0) == Orig)
+      return;
     return getUsefulBitsFromOrWithShiftedReg(SDValue(UserNode, 0), UsefulBits,
                                              Depth);
   case AArch64::BFMWri:
   case AArch64::BFMXri:
     return getUsefulBitsFromBFM(SDValue(UserNode, 0), Orig, UsefulBits, Depth);
 
   case AArch64::STRBBui:
   case AArch64::STURBBi:
     if (UserNode->getOperand(0) != Orig)
       return;
     UsefulBits &= APInt(UsefulBits.getBitWidth(), 0xff);
     return;
 
   case AArch64::STRHHui:
   case AArch64::STURHHi:
     if (UserNode->getOperand(0) != Orig)
       return;
     UsefulBits &= APInt(UsefulBits.getBitWidth(), 0xffff);

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

+; RUN: llc < %s -mtriple=aarch64-unknown-linux-gnu -stop-after=instruction-select -o - | FileCheck %s
+; ModuleID = '<stdin>'
+source_filename = "<stdin>"
+target datalayout = "e-m:e-i8:8:32-i16:16:32-i64:64-i128:128-n32:64-S128"
+target triple = "aarch64-unknown-linux-gnu"
+
+@e = dso_local local_unnamed_addr global i32 0, align 4
+
+; CHECK-LABEL: name: main
+; CHECK-NOT: ORRWrs undef renamable $w8, undef renamable $w8, 73
+; Function Attrs: norecurse
+define dso_local i32 @main() local_unnamed_addr {
+entry:
+  %0 = load i32, i32* @e, align 4
+  %shr = lshr i32 %0, 9
+  %or = or i32 %shr, %0
+  %shr.1 = lshr i32 %or, 9
+  %or.1 = or i32 %shr.1, %or
+  %shr.2 = lshr i32 %or.1, 9
+  %or.2 = or i32 %shr.2, %or.1
+  %shr.3 = lshr i32 %or.2, 9
+  %or.3 = or i32 %shr.3, %or.2
+  %shr.4 = lshr i32 %or.3, 9
+  %or.4 = or i32 %shr.4, %or.3
+  %shr.5 = lshr i32 %or.4, 9
+  %or.5 = or i32 %shr.5, %or.4
+  %shr.6 = lshr i32 %or.5, 9
+  %or.6 = or i32 %shr.6, %or.5
+  %shr.7 = lshr i32 %or.6, 9
+  %or.7 = or i32 %shr.7, %or.6
+  %shr.8 = lshr i32 %or.7, 9
+  %or.8 = or i32 %shr.8, %or.7
+  %shr.9 = lshr i32 %or.8, 9
+  %or.9 = or i32 %shr.9, %or.8
+  %shr.10 = lshr i32 %or.9, 9
+  %or.10 = or i32 %shr.10, %or.9
+  store i32 %or.10, i32* @e, align 4
+  ret i32 0
+}
+