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

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

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

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

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

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

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

[XinWang10]

llvm/lib/CodeGen/HardwareLoops.cpp

330

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/lib/CodeGen/HardwareLoops.cpp

bool HardwareLoopsImpl::TryConvertLoop(Loop *L, LLVMContext &Ctx) {
 
   if (!Opts.Force &&
       !TTI.isHardwareLoopProfitable(L, SE, AC, TLI, HWLoopInfo)) {
     reportHWLoopFailure("it's not profitable to create a hardware-loop",
                         "HWLoopNotProfitable", ORE, L);
     return false;
   }
 
+  assert(
+      Opts.Bitwidth.has_value() && Opts.Decrement.has_value() &&
+      "Need to specify hardware-loop-counter-bitwidth and "
+      "hardware-loop-decrement=1 together when specify force-hardware-loops!");
+
   // Allow overriding of the counter width and loop decrement value.
   if (Opts.Bitwidth.has_value()) {
     HWLoopInfo.CountType = IntegerType::get(Ctx, Opts.Bitwidth.value());
   }
 
   if (Opts.Decrement.has_value())
     HWLoopInfo.LoopDecrement =
       ConstantInt::get(HWLoopInfo.CountType, Opts.Decrement.value());

llvm/test/Transforms/HardwareLoops/input-error.ll

+; RUN: not not opt -passes='hardware-loops<force-hardware-loops>' -S %s 2> %t.err
+; RUN: FileCheck --check-prefix=CHECK < %t.err %s
+; RUN: not not opt -passes='hardware-loops<force-hardware-loops;hardware-loop-counter-bitwidth=32>' -S %s 2> %t.err
+; RUN: FileCheck --check-prefix=CHECK-NODECREMENT < %t.err %s
+; RUN: not not opt -passes='hardware-loops<force-hardware-loops;hardware-loop-decrement=1>' -S %s 2> %t.err
+; RUN: FileCheck --check-prefix=CHECK-NOBITWIDTH < %t.err %s
+
+define void @while_lt(i32 %i, i32 %N, ptr nocapture %A) {
+; CHECK: "Need to specify hardware-loop-counter-bitwidth and " "hardware-loop-decrement=1 together!"
+; CHECK-NODECREMENT: "Need to specify hardware-loop-counter-bitwidth and " "hardware-loop-decrement=1 together!"
+; CHECK-NOBITWIDTH: "Need to specify hardware-loop-counter-bitwidth and " "hardware-loop-decrement=1 together!"
+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
+}