[PATCH] D54882: [AMDGPU] Add sdwa support for ADD|SUB U64 decomposed Pseudos

[Ron Lieberman via Phabricator via llvm-commits]

ronlieb added a comment.

In D54882#1308240 <https://reviews.llvm.org/D54882#1308240>, @rampitec wrote:

> Essentially this is a limited version of shrinking. So I have several questions:
>
> 1. Why not to run shrink pass before sdwa instead?


I tried adding Shrink pass before PeepholeSDWA and observed 88 lit test failures.
i tried moving Shrink pass before Peephole SDWA and observed 25 lit test failures

> 2. If we want to shrink individual instructions, should not we just add SIInstrInfo::shrink() interface and call it from SIShrinkInstructions as well?

not sure ...

> 3. A the very least checks needs to use SIInstrInfo::canShrink() instead of many and insufficient checks here. For instance this code does not check for valid register classes which SIInstrInfo::canShrink() does. We really need to stop cloning that code.

Great suggestion, i added calls to canShrink to replace the exsisting approach. Thanks for pointing that out.

> 4. In general SIInstrInfo::canShrink() should be extended to handle OpSel (via SIInstrInfo::hasModifiers()).

I am not really trying to handle OpSel, and i have modified the current patch to preclude it. We could open another defect to look into this later.

> 5. SIInstrInfo::canShrink() shall call SIInstrInfo::hasVALU32BitEncoding() to check for presence of target instruction.

I added this to canShrink and i think it helps clean up the patch, good suggestion.

New patch coming soon...
still need to construct an MIR test for negative testing.


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

https://reviews.llvm.org/D54882