[PATCH] D56002: [AMDGPU] Fix a weird WWM intrinsic issue.

[Nicolai Hähnle via Phabricator via llvm-commits]

nhaehnle added a comment.

Interesting.

The only user of `canReadVGPR` is `addUsersToMoveToVALUWorklist`. Since the intended semantics of `canReadVGPR` aren't at all clear from the name, might I suggesting folding it into its only user?

Following the chain further down, `getOpRegClass` also doesn't have particularly clear semantics. It's at least documented in the method's comment, but still: a function which returns an instruction's opcode's intrinsic regclass, but then falls back to the regclass that just happens to be there seems a bit misguided to me.

I think `getOpRegClass` should at most be a convenience function that returns the opcode's regclass if it exists, or nullptr otherwise. From a quick glance, it looks like most (all?) users should be fine with either using an opcode's intrinsic regclass (e.g. in `getOpSize`) or, more commonly, with just taking the `MachineOperand`'s register class.

In the case of `addUsersToMoveToVALUWorklist`, the code should just check the users' opcode's intrinsic regclass; if it exists, add the user based on whether the regclass of the opcode has VGPRs (if it doesn't, we need to move-to-VALU, i.e. add to worklist). If it doesn't exist, IMO we should just add to the worklist unconditionally, and handle the fallout (if any) in the main moveToVALU method. That way, the more complex logic is centered in one place.

I'd appreciate if you could help cleanup the technical debt here on those points...


Repository:
  rL LLVM

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

https://reviews.llvm.org/D56002