[PATCH] D117909: [AMDGPU] Remove cndmask from readsExecAsData

[Jay Foad via Phabricator via llvm-commits]

foad added inline comments.


================
Comment at: llvm/lib/Target/AMDGPU/SIInstrInfo.cpp:133
 
 static bool readsExecAsData(const MachineInstr &MI) {
   if (MI.isCompare())
----------------
ruiling wrote:
> The function name is confusing, all these instructions access EXEC just as normal vector instructions. One cause of the failure @sebastian-ne showed in D116053 is we are dropping convergent attribute when lowering amdgcn.icmp into V_CMP. I am not sure if there is any other reason.  I think may be a right fix for the issue is define convergent version of V_CMP, I am not sure whether it sounds reasonable? Please also add some comments this is just workaround for the issue that we are lowering convergent icmp into normal V_CMP. We should fix it some other way.
> I think may be a right fix for the issue is define convergent version of V_CMP, I am not sure whether it sounds reasonable?

Yes, sounds good to me. There is also a related problem in that the "convergent" attribute in IR prevents an instruction from being sunk into an "if" but it does not prevent it from being hoisted out of an "if". For ballot we need both. I'm not sure if the MachineIR convergent flag does both.


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

https://reviews.llvm.org/D117909