[PowerPC] Hardware Loop branch instruction's condition may not be icmp

[shchenz]

Summary

This is for https://bugs.llvm.org/show_bug.cgi?id=42492

In patch for https://reviews.llvm.org/D63477, it assumes that all HardwareLoops loop branch instruction's condition is a icmp. This should be wrong.

For case in https://bugs.llvm.org/show_bug.cgi?id=42492 , branch instruction's condition is not a icmp, it is a or instead.

[samparker]

Please could you add an opt test for the hardware-loops too? This looks like a corner case that we're not getting generic coverage on.

[shchenz]

@samparker Thanks for your comment, Sam. Updated the patch. I use the same source in both opt test and llc test, since the testing point is not the same, hope this is ok.

[samparker]

Thanks. LGTM

[hfinkel]

In D64124#1568593, @samparker wrote:

Thanks. LGTM

LGTM too, just some comments about the test.

llvm/test/CodeGen/PowerPC/pr42492.ll

2

You don't need 'REQUIRES: asserts' here. 'REQUIRES: asserts' is only necessary if asserts are necessary for the test to pass, not if asserts is required for the test to fail. The test still might fail anyway (e.g., it might segfault).

Also, in this case, it seems as though the test can test for correct behavior (not just the absence of a crash), and so please add some CHECK lines to check for the expected output in this case.

[shchenz]

address review comments about test case.

llvm/lib/Transforms/Scalar/LoopStrengthReduce.cpp

for (const IVStrideUse &U : IU) {
     // with rather than just N or i, so we can consider the register
     // requirements for both N and i at the same time. Limiting this code to
     // equality icmps is not a problem because all interesting loops use
     // equality icmps, thanks to IndVarSimplify.
     if (ICmpInst *CI = dyn_cast<ICmpInst>(UserInst))
       if (CI->isEquality()) {
         // If CI can be saved in some target, like replaced inside hardware loop
         // in PowerPC, no need to generate initial formulae for it.
-        if (SaveCmp && CI == cast<ICmpInst>(ExitBranch->getCondition()))
+        if (SaveCmp && CI == dyn_cast<ICmpInst>(ExitBranch->getCondition()))
           continue;
         // Swap the operands if needed to put the OperandValToReplace on the
         // left, for consistency.
         Value *NV = CI->getOperand(1);
         if (NV == U.getOperandValToReplace()) {
           CI->setOperand(1, CI->getOperand(0));
           CI->setOperand(0, NV);
           NV = CI->getOperand(1);

llvm/test/CodeGen/PowerPC/pr42492.ll

+; REQUIRES: asserts
+; RUN: llc -mtriple=powerpc64-unknown-linux-gnu -mcpu=pwr9 < %s | FileCheck %s
+
+; Check we don't assert.
+; CHECK-LABEL: f:
+define void @f(i8*, i8*, i64*) {
+  %4 = icmp eq i8* %0, %1
+  br i1 %4, label %9, label %5
+
+5:                                                ; preds = %3
+  %6 = getelementptr inbounds i64, i64* %2, i64 1
+  %7 = load i64, i64* %6, align 8
+  br label %10
+
+8:                                                ; preds = %10
+  store i64 %14, i64* %6, align 8
+  br label %9
+
+9:                                                ; preds = %8, %3
+  ret void
+
+10:                                               ; preds = %5, %10
+  %11 = phi i64 [ %7, %5 ], [ %14, %10 ]
+  %12 = phi i32 [ 0, %5 ], [ %15, %10 ]
+  %13 = phi i8* [ %0, %5 ], [ %16, %10 ]
+  %14 = shl nsw i64 %11, 4
+  %15 = add nuw nsw i32 %12, 1
+  %16 = getelementptr inbounds i8, i8* %13, i64 1
+  %17 = icmp ugt i32 %12, 14
+  %18 = icmp eq i8* %16, %1
+  %19 = or i1 %18, %17
+  br i1 %19, label %8, label %10
+}