[Openmp-commits] [PATCH] D146277: Handle the unexpected inputs for pass HardwareLoops

[Wang, Xin via Phabricator via Openmp-commits]

XinWang10 added inline comments.


================
Comment at: llvm/lib/CodeGen/HardwareLoops.cpp:330
 
+  assert(
+      Opts.Bitwidth.has_value() && Opts.Decrement.has_value() &&
----------------
samparker wrote:
> XinWang10 wrote:
> > samparker wrote:
> > > samparker wrote:
> > > > XinWang10 wrote:
> > > > > fhahn wrote:
> > > > > > 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.
> > > > > 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.
> > > > So, `hardware-loop-counter-bitwidth` defaults to 32. so how about we set the default value of `HardwareLoopInfo.CountType` to `Int32Ty`?
> > > 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.
> > > 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.
> Please see D147042 as an illustration of what I meant. Would it solve these problems?
> 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.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D146277/new/

https://reviews.llvm.org/D146277