[llvm-commits] [PATCH] X86: Add support for GATHER intrinsics of X86 AVX2, please review

Tue Jun 26 10:08:59 PDT 2012

Hi Manman,

A few questions.

> --- lib/Target/X86/AsmParser/X86AsmParser.cpp	(revision 159034)
> +++ lib/Target/X86/AsmParser/X86AsmParser.cpp	(working copy)
> @@ -916,15 +916,16 @@
>  
>    // If we have both a base register and an index register make sure they are
>    // both 64-bit or 32-bit registers.
> +  // To support VSIB, IndexReg can be 128-bit or 256-bit registers.
>    if (BaseReg != 0 && IndexReg != 0) {
>      if (X86MCRegisterClasses[X86::GR64RegClassID].contains(BaseReg) &&
> -        !X86MCRegisterClasses[X86::GR64RegClassID].contains(IndexReg) &&
> +        X86MCRegisterClasses[X86::GR32RegClassID].contains(IndexReg) &&
>          IndexReg != X86::RIZ) {
>        Error(IndexLoc, "index register is 32-bit, but base register is 64-bit");
>        return 0;
>      }
>      if (X86MCRegisterClasses[X86::GR32RegClassID].contains(BaseReg) &&
> -        !X86MCRegisterClasses[X86::GR32RegClassID].contains(IndexReg) &&
> +        X86MCRegisterClasses[X86::GR64RegClassID].contains(IndexReg) &&
>          IndexReg != X86::EIZ){
>        Error(IndexLoc, "index register is 64-bit, but base register is 32-bit");
>        return 0;

Does this change the diagnostic reporting for instructions unrelated to the patch? The conditionals are going from fairly general to more specific and this is a very general purpose function. I'm worried that we'll have some cases that used to get the specific diagnostics above will now get the accurate, but much less helpful, "invalid operand" diagnostic (or worse, no diagnostic at all).

> --- lib/Target/X86/MCTargetDesc/X86MCCodeEmitter.cpp	(revision 159034)
> +++ lib/Target/X86/MCTargetDesc/X86MCCodeEmitter.cpp	(working copy)
> @@ -621,7 +621,9 @@
>        VEX_X = 0x0;
>  
>      if (HasVEX_4VOp3)
> -      VEX_4V = getVEXRegisterEncoding(MI, X86::AddrNumOperands+1);
> +      // CurOp points to start of the MemoryOperand.
> +      //   src1(ModR/M), MemAddr, src3(VEX_4V)
> +      VEX_4V = getVEXRegisterEncoding(MI, CurOp+X86::AddrNumOperands);
> 

Can you elaborate a bit on what's going on here? Was this code incorrect before and this is a bug fix? It's not immediately obvious to me that this doesn't change the behavior for instructions that pre-date this patch, and I'm not sure if it should or not.

> +
> +    // Check whether we are handling VSIB addressing mode for GATHER.
> +    // If sibIndex was set to SIB_INDEX_NONE, index offset is 4 and
> +    // we should use SIB_INDEX_XMM4|YMM4 for VSIB.
> +    uint32_t Opcode = mcInst.getOpcode();
> +    bool IsGather = (Opcode == X86::VGATHERDPDrm ||
> +                     Opcode == X86::VGATHERQPDrm ||
> +                     Opcode == X86::VGATHERDPSrm ||
> +                     Opcode == X86::VGATHERQPSrm);
> +    bool IsGatherY = (Opcode == X86::VGATHERDPDYrm ||
> +                      Opcode == X86::VGATHERQPDYrm ||
> +                      Opcode == X86::VGATHERDPSYrm ||
> +                      Opcode == X86::VGATHERQPSYrm);
> +    if (IsGather || IsGatherY) {
> +      unsigned IndexOffset = insn.sibIndex -
> +                         (insn.addressSize == 8 ? SIB_INDEX_RAX:SIB_INDEX_EAX);
> +      SIBIndex IndexBase = IsGatherY ? SIB_INDEX_YMM0 : SIB_INDEX_XMM0;
> +      insn.sibIndex = (SIBIndex)(IndexBase + 
> +                           (insn.sibIndex == SIB_INDEX_NONE ? 4 : IndexOffset));
> +    }
> +

Per our in-person conversation, you're right that this feels a bit hacky. I don't think it's a big enough issue to stop the patch from going in, but I think it's worth a bit of investigation to see if there's a clean way to define these instructions so that the sibIndex comes in to this routine defined properly already.

-Jim

On Jun 25, 2012, at 10:50 AM, Manman Ren <mren at apple.com> wrote:

> 
> ping
> <avx2.llvm.2.patch>
> 
>> 
>> 
>> -----Original Message-----
>> From: llvm-commits-bounces at cs.uiuc.edu [mailto:llvm-commits-bounces at cs.uiuc.edu] On Behalf Of Manman Ren
>> Sent: Saturday, June 23, 2012 01:12
>> To: Commit Messages and Patches for LLVM
>> Subject: [llvm-commits] [PATCH] X86: Add support for GATHER intrinsics of X86 AVX2, please review
>> 
>> 
>> Support the following intrinsics:
>>  _mm_mask_i32gather_pd, _mm256_mask_i32gather_pd, _mm_mask_i64gather_pd
>>  _mm256_mask_i64gather_pd, _mm_mask_i32gather_ps, _mm256_mask_i32gather_ps
>>  _mm_mask_i64gather_ps, _mm256_mask_i64gather_ps
>> 
>> There are two places which I am not sure about:
>> 1> Is a customized ISel needed for GATHER?
>>   I can't figure out how to select the address operands for GATHER intrinsics
>>   in a .td file, given that index and scale are passed in as parameters.
>>   gather(<2 x double> %a0, i8* %base, <2 x i64> %idx, <2 x double> %mask, i8 scale) 
>> 
>> 2> Is there a better way to modify disassembler to handle VSIB addressing mode?
>>   The existing disassembler assumes index register is encoded for SIB only.
>>   However VSIB has a different mapping for the index registers.
>>   For example: index field of 100 means VR4 or VR12 for VSIB, and it means
>>                no index register for SIB.
>>   We don't know whether it is VSIB or SIB before the instruction ID is decoded.
>>   It looks to me that X86::VGATHER is not exposed to readSIB in X86DisassemblerDecoder.c
>>   The attached patch checks whether it is VSIB during translation and if yes,
>>   it will update index register to the correct value according to VSIB.
>> 
>> File modified:
>> test/MC/X86/x86_64-avx-encoding.s
>> test/MC/Disassembler/X86/simple-tests.txt
>> test/CodeGen/X86/avx2-intrinsics-x86.ll
>> include/llvm/IntrinsicsX86.td
>> utils/TableGen/EDEmitter.cpp
>> utils/TableGen/X86RecognizableInstr.cpp
>> lib/Target/X86/X86InstrInfo.td
>> lib/Target/X86/AsmParser/X86AsmParser.cpp
>> lib/Target/X86/X86ISelDAGToDAG.cpp
>> lib/Target/X86/X86InstrSSE.td
>> lib/Target/X86/MCTargetDesc/X86MCCodeEmitter.cpp
>> lib/Target/X86/Disassembler/X86DisassemblerDecoder.h
>> lib/Target/X86/Disassembler/X86Disassembler.cpp
>> 
>> Comments are appreciated.
>> 
>> Thanks,
>> Manman
>> ---------------------------------------------------------------------
>> Intel Israel (74) Limited
>> 
>> This e-mail and any attachments may contain confidential material for
>> the sole use of the intended recipient(s). Any review or distribution
>> by others is strictly prohibited. If you are not the intended
>> recipient, please contact the sender and delete all copies.
>> 
> 
> _______________________________________________
> llvm-commits mailing list
> llvm-commits at cs.uiuc.edu
> http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20120626/843719e4/attachment.html>