[PATCH] D98464: [GlobalISel] Add G_SBFX + G_UBFX (bitfield extraction opcodes)

[Matt Arsenault via Phabricator via llvm-commits]

arsenm added a comment.

In D98464#2622698 <https://reviews.llvm.org/D98464#2622698>, @aemerson wrote:

> If we have to allow variable width operands for these, it doesn't really help AArch64 that much. We have to match the constant pattern to a target specific opcode, and then lower the rest of these to shifts by default. I can see the benefit of AMDGPU, but with variable extracts this just puts us back where we started for AArch64.



In D98464#2627521 <https://reviews.llvm.org/D98464#2627521>, @aemerson wrote:

> In D98464#2622885 <https://reviews.llvm.org/D98464#2622885>, @paquette wrote:
>
>> I think something like this would work post-legalization:
>>
>>   // We'd need these functions...
>>   if (isBeforeLegalizer() || !isLegal(...))
>>     return false;
>>   
>>   Register LshrLHS, LshrRHS;
>>   int64_t Mask;
>>   if (!mi_match(Reg, MRI,
>>                 m_GAnd(m_GLShr(m_Reg(LshrLHS), m_Reg(LshrRHS)), m_ICst(Mask))))
>>     return false;
>>   if (/*Mask isn't a mask...*/)
>>     return false;
>>   
>>   if (TLI.allowsRegisterExtractOps(/*...*/)) {
>>     // ... Do stuff ...
>>     return true;
>>   }
>>   
>>   // Need immediates for LSB + width.
>>   int64_t LshrImm;
>>   if (!mi_match(LshrRHS, MRI, m_ICst(LshrImm)))
>>     return false;
>>   
>>   // ... Do stuff ...
>>   return true;
>
> So we form these post-legalize only if we can see that the sources are constant for arm64. Sounds reasonable to me. @arsenm that ok for AMDGPU?

Yes, forming them only if the target supports the variable case makes sense (although also having the legalize back to shifts path would be nice for consistency)


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

https://reviews.llvm.org/D98464