[PATCH] D81085: [AMDGPU/MemOpsCluster] Implement new heuristic for computing max mem ops cluster size

[Mahesha S via Phabricator via llvm-commits]

hsmhsm added a comment.

In D81085#2084501 <https://reviews.llvm.org/D81085#2084501>, @foad wrote:

> > Looks like this breaks check-llvm: http://45.33.8.238/linux/19836/step_12.txt
>
> @hsmhsm you can reproduce the failures if you run the tests under valgrind with `llvm-lit --vg`. Adding an `assert(Width > 0)` just after MachineScheduler calls getMemOperandsWithOffsetWidth would make the failure even more obvious.
>
> The problem is: first you committed D80946 <https://reviews.llvm.org/D80946> which returns `Width` from all cases in `SIInstrInfo::getMemOperandsWithOffsetWidth`. Then I committed D74035 <https://reviews.llvm.org/D74035>, which was written earlier, which adds a new `isMIMG` case but does not set `Width`.


Hi @foad,

Ok, this is interesting. Anyway, so, we need to make sure that `SIInstrInfo::getMemOperandsWithOffsetWidth()` will compute `width` for all cases. So, for the new case, that you have added, will you going to sumbit a patch which make sure that `width` is computed for this new case as well?

In D81085#2084501 <https://reviews.llvm.org/D81085#2084501>, @foad wrote:

> > Looks like this breaks check-llvm: http://45.33.8.238/linux/19836/step_12.txt
>
> @hsmhsm you can reproduce the failures if you run the tests under valgrind with `llvm-lit --vg`. Adding an `assert(Width > 0)` just after MachineScheduler calls getMemOperandsWithOffsetWidth would make the failure even more obvious.
>
> The problem is: first you committed D80946 <https://reviews.llvm.org/D80946> which returns `Width` from all cases in `SIInstrInfo::getMemOperandsWithOffsetWidth`. Then I committed D74035 <https://reviews.llvm.org/D74035>, which was written earlier, which adds a new `isMIMG` case but does not set `Width`.





Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D81085