[PATCH] D52559: [AMDGPU] Divergence driven instruction selection. Shift operations.

[Stanislav Mekhanoshin via Phabricator via llvm-commits]

rampitec requested changes to this revision.
rampitec added inline comments.
This revision now requires changes to proceed.


================
Comment at: lib/Target/AMDGPU/VOP3Instructions.td:587
+    VOP3e_si <op, !cast<VOP3_Pseudo>(NAME).Pfl> {
+      // Hack to stop printing _e64
+      VOP3_Pseudo ps = !cast<VOP3_Pseudo>(NAME);
----------------
alex-t wrote:
> rampitec wrote:
> > alex-t wrote:
> > > rampitec wrote:
> > > > Does it really belong to this patch?
> > > Sure it does. As soon as I start selecting this _b64  the LIT tests failed because of the odd "_e64" suffix printed by AMDGPUInstrPrinter. It prints it for all with VOP3 flag.
> > > The flag was not set before this change because the instructions were created in moveToVALU.  
> > > I filed the bug https://bugs.llvm.org/show_bug.cgi?id=39086 for adding the flag to indicate that the instruction does not have 32bit encoding.
> > > 
> > How can it be possible for moveToVALU to create a VOP3 instruction without a VOP3 flag?
> > VOP3_Pseudo sets VOP3 flag. InstSI copies it into TSFlags. VOP3_Real copies TSFlags from a pseudo.
> InstSI copies VOP3 to TSFlags and your code checks exactly TSFlags & SIInstrFlags::VOP3
So the explanation is wrong. It has VOP3 in TSFlags with or without your changes. It is worth nothing where the instruction is created. The suffix was not printed because the control did not even come to the AMDGPUInstPrinter::printVOPDst, and it did not trigger because these instructions did not have VOPDstOperand in the td. Now what you have changed is the operand definition. What you need to fix the problem instead of the hack is to return DstRC back to the RegisterOperand<VReg_64> in the profile.


================
Comment at: lib/Target/AMDGPU/VOP3Instructions.td:392
+let SubtargetPredicate = isVI, Predicates = [isVI] in {
+def V_LSHLREV_B64 : VOP3Inst <"v_lshlrev_b64", VOP_PAT_GEN<VOP3_Profile<VOP_I64_I32_I64>>, shl>;
+def V_LSHRREV_B64 : VOP3Inst <"v_lshrrev_b64", VOP_PAT_GEN<VOP3_Profile<VOP_I64_I32_I64>>, srl>;
----------------
It sounds like from your explanation below and the logic of the getVOP3Pat this will create a bogus pattern with wrong operand order. Only one pattern shall exist for (shl i64:x, i32:y) and it seems to be the pattern below. At best this one will never match.


https://reviews.llvm.org/D52559