[PATCH] D108022: [AMDGPU] Skip pseudo MIs in hazard recognizer

[Austin Kerbow via Phabricator via llvm-commits]

kerbowa added a comment.

In D108022#2947383 <https://reviews.llvm.org/D108022#2947383>, @cdevadas wrote:

> In D108022#2944003 <https://reviews.llvm.org/D108022#2944003>, @rampitec wrote:
>
>> In D108022#2943841 <https://reviews.llvm.org/D108022#2943841>, @arsenm wrote:
>>
>>> In D108022#2943806 <https://reviews.llvm.org/D108022#2943806>, @cdevadas wrote:
>>>
>>>> In D108022#2943730 <https://reviews.llvm.org/D108022#2943730>, @arsenm wrote:
>>>>
>>>>> I think you're missing the check in AdvanceCycle
>>>>
>>>> There was a call to getNumWaitStates in AdvanceCycle. I presumed a zero waitstate here is equivalent to an early return.
>>>> Isn't it?
>>>> https://github.com/llvm/llvm-project/blob/main/llvm/lib/Target/AMDGPU/GCNHazardRecognizer.cpp#L365
>>>
>>> I think the early return on isMetaInstruction avoids pushing the instruction into EmittedInstrs
>>
>> I think AdvanceCycle shall do the same as with isMetaInstruction() if getNumWaitStates() returned 0.
>
> The AdvanceCycle will be called by both Post-RA scheduler and hazard recognizer. I am beginning to think that the early return is necessary to prevent adding it into EmittedInstrs.
> Alternatively, how about return early when NumWaitStates is zero?
> https://github.com/llvm/llvm-project/blob/main/llvm/lib/Target/AMDGPU/GCNHazardRecognizer.cpp#L366

Right, it matters in the post-RA scheduler. For example, if you replace the KILL with WAVE_BARRIER in the test hazard-kill.mir, the scheduler doesn't see the hazard.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D108022