Handle the unexpected inputs for pass HardwareLoops

[XinWang10]

Summary

For a function TryConvertLoop in pass HardwareLoops, wrong input arguments will
lead to crash. There will be 3 cases.
In line 342, compiler want to get something from
HWLoopInfo.CountType, which depends on if argument Bitwidth is given, if not,
will crash.
In Function isHardwareLoopCandidate, it dereference CountType too.
In Function InsertLoopDec, it dereference LoopDecrement.
They all could lead to crash. This patch add condition to this pass, when we meet unexpected inputs then skip
the pass.

[samparker]

Okay, thanks.

[fhahn]

llvm/lib/CodeGen/HardwareLoops.cpp

330 ↗

(On Diff #506012)

It shouldn't be possible to trigger an assert when passing arguments to the pass. If there's an invalid combination, it should be handled gracefully, e.g. bailing out without doing anything (together with a debug message perhaps) or using a sane default value if it makes sense.

[fhahn]

Looks like multiple existing tests are triggering the assert and are failing.

[XinWang10]

• fix lit fails

[XinWang10]

• adjust clang-format

[XinWang10]

try rebase

[XinWang10]

• add the raw blank

[XinWang10]

• keep same to raw file

[XinWang10]

• keep same to raw file

[XinWang10]

llvm/lib/CodeGen/HardwareLoops.cpp

330 ↗

(On Diff #506012)

I'm not familiar with this pass so I'm not sure how to correctly handle the unexpected input, maybe someone can refine it later? But for now, the wrong input is unacceptable b/c it will lead to crash.

[XinWang10]

• change assert to report_fatal_error

[XinWang10]

• delete report_fata_error and use return to skip

[samparker]

llvm/lib/CodeGen/HardwareLoops.cpp

330 ↗

(On Diff #506012)

So, hardware-loop-counter-bitwidth defaults to 32. so how about we set the default value of HardwareLoopInfo.CountType to Int32Ty?

[XinWang10]

• give it default values

[XinWang10]

• remove old code

[samparker]

llvm/lib/CodeGen/HardwareLoops.cpp

330 ↗

(On Diff #506012)

Sorry, I didn't mean setting it here. struct HardwareLoopInfo is declared in llvm/include/llvm/Analysis/TargetTransformInfo.h, and I think giving the member CountType a default value should work.

[XinWang10]

• narrow down the condition

[XinWang10]

narrow down the condition

[XinWang10]

llvm/lib/CodeGen/HardwareLoops.cpp

330 ↗

(On Diff #506012)

Sorry, I didn't mean setting it here. struct HardwareLoopInfo is declared in llvm/include/llvm/Analysis/TargetTransformInfo.h, and I think giving the member CountType a default value should work.

Yes, I adjust the code. What I want to point out is that there could be 3 crash situation here, 1 in line 337 in raw file, one in isHardwareLoopCandidate which also use CountType, another in InsertLoopDec which use LoopDecrement.

[samparker]

llvm/lib/CodeGen/HardwareLoops.cpp

330 ↗

(On Diff #506012)

Please see D147042 as an illustration of what I meant. Would it solve these problems?

[XinWang10]

llvm/lib/CodeGen/HardwareLoops.cpp

330 ↗

(On Diff #506012)

Please see D147042 as an illustration of what I meant. Would it solve these problems?

Oh, I see. It's much elegant. Thanks for your explaination, will update.

[XinWang10]

• use default constructor to give it a default value

[samparker]

Many thanks!

[fhahn]

LGTM, thanks for the updates!

[XinWang10]

Thanks for your review~

llvm/include/llvm/Analysis/TargetTransformInfo.h

return (Ordering == AtomicOrdering::NotAtomic ||
             Ordering == AtomicOrdering::Unordered) &&
            !IsVolatile;
   }
 };
 
 /// Attributes of a target dependent hardware loop.
 struct HardwareLoopInfo {
   HardwareLoopInfo() = delete;
-  HardwareLoopInfo(Loop *L) : L(L) {}
+  HardwareLoopInfo(Loop *L);
   Loop *L = nullptr;
   BasicBlock *ExitBlock = nullptr;
   BranchInst *ExitBranch = nullptr;
   const SCEV *ExitCount = nullptr;
   IntegerType *CountType = nullptr;
   Value *LoopDecrement = nullptr; // Decrement the loop counter by this
                                   // value in every iteration.
   bool IsNestingLegal = false;    // Can a hardware loop be a parent to

llvm/lib/Analysis/TargetTransformInfo.cpp

IntrinsicCostAttributes::IntrinsicCostAttributes(Intrinsic::ID Id, Type *RTy,
                                                  FastMathFlags Flags,
                                                  const IntrinsicInst *I,
                                                  InstructionCost ScalarCost)
     : II(I), RetTy(RTy), IID(Id), FMF(Flags), ScalarizationCost(ScalarCost) {
   ParamTys.insert(ParamTys.begin(), Tys.begin(), Tys.end());
   Arguments.insert(Arguments.begin(), Args.begin(), Args.end());
 }
 
+HardwareLoopInfo::HardwareLoopInfo(Loop *L) : L(L) {
+  // Match default options:
+  // - hardware-loop-counter-bitwidth = 32
+  // - hardware-loop-decrement = 1
+  CountType = Type::getInt32Ty(L->getHeader()->getContext());
+  LoopDecrement = ConstantInt::get(CountType, 1);
+}
+
 bool HardwareLoopInfo::isHardwareLoopCandidate(ScalarEvolution &SE,
                                                LoopInfo &LI, DominatorTree &DT,
                                                bool ForceNestedLoop,
                                                bool ForceHardwareLoopPHI) {
   SmallVector<BasicBlock *, 4> ExitingBlocks;
   L->getExitingBlocks(ExitingBlocks);
 
   for (BasicBlock *BB : ExitingBlocks) {

llvm/test/Transforms/HardwareLoops/unexpected-inputs.ll

+; RUN: opt -passes='hardware-loops<force-hardware-loops>' -S %s -o - | FileCheck %s --check-prefix=CHECK
+; RUN: opt -passes='hardware-loops<force-hardware-loops;hardware-loop-decrement=1>' -S %s -o - | FileCheck %s --check-prefix=CHECK
+; RUN: opt -passes='hardware-loops<force-hardware-loops;hardware-loop-counter-bitwidth=32>' -S %s -o - | FileCheck %s --check-prefix=CHECK
+
+define void @while_lt(i32 %i, i32 %N, ptr nocapture %A) {
+; CHECK-LABEL: @while_lt(
+; CHECK-NEXT:  entry:
+; CHECK-NEXT:    [[CMP4:%.*]] = icmp ult i32 [[I:%.*]], [[N:%.*]]
+; CHECK-NEXT:    br i1 [[CMP4]], label [[WHILE_BODY_PREHEADER:%.*]], label [[WHILE_END:%.*]]
+; CHECK:       while.body.preheader:
+; CHECK-NEXT:    [[TMP0:%.*]] = sub i32 [[N]], [[I]]
+; CHECK-NEXT:    call void @llvm.set.loop.iterations.i32(i32 [[TMP0]])
+; CHECK-NEXT:    br label [[WHILE_BODY:%.*]]
+; CHECK:       while.body:
+; CHECK-NEXT:    [[I_ADDR_05:%.*]] = phi i32 [ [[INC:%.*]], [[WHILE_BODY]] ], [ [[I]], [[WHILE_BODY_PREHEADER]] ]
+; CHECK-NEXT:    [[ARRAYIDX:%.*]] = getelementptr inbounds i32, ptr [[A:%.*]], i32 [[I_ADDR_05]]
+; CHECK-NEXT:    store i32 [[I_ADDR_05]], ptr [[ARRAYIDX]], align 4
+; CHECK-NEXT:    [[INC]] = add nuw i32 [[I_ADDR_05]], 1
+; CHECK-NEXT:    [[TMP1:%.*]] = call i1 @llvm.loop.decrement.i32(i32 1)
+; CHECK-NEXT:    br i1 [[TMP1]], label [[WHILE_BODY]], label [[WHILE_END]]
+; CHECK:       while.end:
+; CHECK-NEXT:    ret void
+entry:
+  %cmp4 = icmp ult i32 %i, %N
+  br i1 %cmp4, label %while.body, label %while.end
+
+while.body:
+  %i.addr.05 = phi i32 [ %inc, %while.body ], [ %i, %entry ]
+  %arrayidx = getelementptr inbounds i32, ptr %A, i32 %i.addr.05
+  store i32 %i.addr.05, ptr %arrayidx, align 4
+  %inc = add nuw i32 %i.addr.05, 1
+  %exitcond = icmp eq i32 %inc, %N
+  br i1 %exitcond, label %while.end, label %while.body
+
+while.end:
+  ret void
+}