This is an archive of the discontinued LLVM Phabricator instance.

Handle the unexpected inputs for pass HardwareLoops
ClosedPublic

Authored by XinWang10 on Mar 17 2023, 2:26 AM.

Details

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.

Diff Detail

Event Timeline

XinWang10 created this revision.Mar 17 2023, 2:26 AM
Herald added a project: Restricted Project. · View Herald TranscriptMar 17 2023, 2:26 AM
Herald added a subscriber: hiraditya. · View Herald Transcript
XinWang10 requested review of this revision.Mar 17 2023, 2:26 AM
Herald added a project: Restricted Project. · View Herald TranscriptMar 17 2023, 2:26 AM
samparker accepted this revision.Mar 17 2023, 3:15 AM

Okay, thanks.

This revision is now accepted and ready to land.Mar 17 2023, 3:15 AM
fhahn added a subscriber: fhahn.Mar 17 2023, 3:33 AM
fhahn added inline comments.
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 requested changes to this revision.Mar 17 2023, 3:34 AM

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

This revision now requires changes to proceed.Mar 17 2023, 3:34 AM
XinWang10 updated this revision to Diff 507597.Mar 22 2023, 9:38 PM
  • fix lit fails
  • adjust clang-format
  • add the raw blank
  • keep same to raw file
  • keep same to raw file
XinWang10 added inline comments.Mar 23 2023, 7:26 PM
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 updated this revision to Diff 507991.Mar 24 2023, 1:33 AM
  • change assert to report_fatal_error
XinWang10 edited the summary of this revision. (Show Details)Mar 24 2023, 2:27 AM
XinWang10 updated this revision to Diff 508460.Mar 26 2023, 7:33 PM
  • delete report_fata_error and use return to skip
XinWang10 edited the summary of this revision. (Show Details)Mar 26 2023, 7:34 PM
XinWang10 retitled this revision from add assert to confirm input is valid for pass HardwareLoops to Handle the unexpected inputs for pass HardwareLoops.Mar 26 2023, 8:07 PM
XinWang10 edited the summary of this revision. (Show Details)Mar 26 2023, 8:30 PM
samparker added inline comments.Mar 27 2023, 4:06 AM
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 updated this revision to Diff 508872.Mar 27 2023, 7:35 PM
  • give it default values
XinWang10 updated this revision to Diff 508873.Mar 27 2023, 7:36 PM
  • remove old code
samparker added inline comments.Mar 28 2023, 2:01 AM
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 updated this revision to Diff 508954.Mar 28 2023, 4:02 AM
  • narrow down the condition
XinWang10 updated this revision to Diff 508956.Mar 28 2023, 4:03 AM

narrow down the condition

XinWang10 edited the summary of this revision. (Show Details)Mar 28 2023, 4:09 AM
XinWang10 added inline comments.
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 added inline comments.Mar 28 2023, 5:30 AM
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 added inline comments.Mar 28 2023, 6:33 PM
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 updated this revision to Diff 509187.Mar 28 2023, 7:05 PM
  • use default constructor to give it a default value
samparker accepted this revision.Mar 29 2023, 2:00 AM

Many thanks!

fhahn accepted this revision.Mar 29 2023, 7:28 AM

LGTM, thanks for the updates!

This revision is now accepted and ready to land.Mar 29 2023, 7:28 AM
This revision was automatically updated to reflect the committed changes.

Thanks for your review~