Add aliases for VAND imm to VBIC ~imm

[rengolin]

Summary

On ARM NEON, VAND with immediate (16/32 bits) is an alias to VBIC ~imm with
the same type size. Adding that logic to the parser, and generating VBIC
instructions from VAND asm files.

Fixes PR20702.

[t.p.northover]

Hi Renato,

Have a razor, I think you've got a yak to shave...

lib/Target/ARM/ARMInstrNEON.td

39

PrintMethod shouldn't be needed since the aliases aren't canonical.

lib/Target/ARM/AsmParser/ARMAsmParser.cpp

1629

Is this right? From the ARM ARM it looks like the set of constants permitted is not invariant under Imm -> ~Imm.

Actually, the code above in isNEONi16Splat is wrong too (which is why your test happens to work anyway):

$ echo "vbic.i16 d10, #0x03ff" | bin/llvm-mc -triple armv7 -show-inst
    vbic.i16 d10, #0x300

2409

This isn't really a validation (which implies it might be invalid). Perhaps an "encodeNEON..."; if so, it should probably go into MCTargetDesc with the others.

[rengolin]

lib/Target/ARM/ARMInstrNEON.td

39

ok

lib/Target/ARM/AsmParser/ARMAsmParser.cpp

1629

Oh god, no! Lazy arse, I am. I meant to invert the value first, but got too excited that my test case passed! Sorry.

2409

encode is a better name, yes. Where in MCTargetDesc you're thinking of?

[t.p.northover]

lib/Target/ARM/AsmParser/ARMAsmParser.cpp

2409

It looks like there's already some utilities for modified NEON values in ARMAddressingModes.h. Just no encode that I could see yet.

[rengolin]

Right, ignoring the code in isNEONi32splat() for now, I just want to make sure everything is in the right place.

I have:

• removed the unnecessary print methods
• moved validateNEON... to ARMAddressingModes.h as encodeNEON...
• added a common isNEON to the same header and inverted the value on ...SplatNot methods

I haven't:

• fixed isNEON...
• added enough tests to make sure a whole range of values are encoded correctly

I don't know if:

• isNEON... is a good name, since that it's now just checking for splat consistency and changing it. Maybe encodeNEON...Value() would be better?

[t.p.northover]

Hi Renato,

The names look reasonable. About the one thing I'd change is not mentioning "Operands" in ARMAddressingModes.h. That's entirely a bike-shedding point though, I wouldn't have mentioned it if you hadn't specifically mentioned names as something you weren't sure of.

Cheers.

Tim.

lib/Target/ARM/AsmParser/ARMAsmParser.cpp

1653–1654

Is this right? I don't know what it's trying to exclude for VBIC, but I'd be slightly surprised if just these 4 aliases also make it work for VAND too.

2434–2435

It'd be nice if there was some consistency between the i16 and i32 variants here (int/int64_t/unsigned/...). It feels like there ought to be a good canonical representation.

lib/Target/ARM/MCTargetDesc/ARMAddressingModes.h

581 ↗

(On Diff #13462)

assert on low bits here, when you get around to fixing the bug?

[rengolin]

Thanks for the review, Tim. I'll refactor the names and types and start working on the acceptable cases and errors.

cheers,
--renato

lib/Target/ARM/AsmParser/ARMAsmParser.cpp

1653–1654

Honestly, I don't know. Until now, it was more like copy&paste than anything else. I'll start paying attention to the edge cases when the names and locations are correct and some of the cases can be dealt with, so it becomes easier to debug and trace the paths.

2434–2435

The reason why one is unsigned and the other is uint64_t is because on the 32-bit case, there's a warning on a comparison with the sign bit in isNEONi32splat. I'll see if I can make it less confusing...

[rengolin]

Many changes since last diff...

• fixed immediate checking
• renamed functions to be consistent
• made all arguments unsigned int
• added more tests for valid and invalid patterns

I believe now it's a lot closer to committing quality.

[rengolin]

Ping

[rengolin]

Ping?

[aadg]

Looks good to me.

[aadg]

This look ok to me.

For some reason, the comment I made in phabricator never made it to the mailing list.

Cheers,

Arnaud

From: [e-mail] [mailto:[e-mail]] On Behalf Of James Molloy
Sent: 25 September 2014 09:41
To: [e-mail]
Cc: James Molloy; LLVM Commits; Tim Northover; Arnaud De Grandmaison
Subject: Re: [PATCH] Add aliases for VAND imm to VBIC ~imm

Adding Arnaud as he is working on pseudo instructions.

[rengolin]

Thanks Arnaud, I got the email... :)

[rengolin]

Committed in r218450.

Thanks!
--renato

lib/Target/ARM/ARMInstrNEON.td

   let PrintMethod = "printNEONModImmOperand";
   let ParserMatchClass = nImmSplatI16AsmOperand;
 }
 def nImmSplatI32AsmOperand : AsmOperandClass { let Name = "NEONi32splat"; }
 def nImmSplatI32 : Operand<i32> {
   let PrintMethod = "printNEONModImmOperand";
   let ParserMatchClass = nImmSplatI32AsmOperand;
 }
+def nImmSplatNotI16AsmOperand : AsmOperandClass { let Name = "NEONi16splatNot"; }
+def nImmSplatNotI16 : Operand<i32> {
+  let PrintMethod = "printNEONModImmOperand";
+  let ParserMatchClass = nImmSplatNotI16AsmOperand;
+}
+def nImmSplatNotI32AsmOperand : AsmOperandClass { let Name = "NEONi32splatNot"; }
+def nImmSplatNotI32 : Operand<i32> {
+  let PrintMethod = "printNEONModImmOperand";
+  let ParserMatchClass = nImmSplatNotI32AsmOperand;
+}
 def nImmVMOVI32AsmOperand : AsmOperandClass { let Name = "NEONi32vmov"; }
 def nImmVMOVI32 : Operand<i32> {
   let PrintMethod = "printNEONModImmOperand";
   let ParserMatchClass = nImmVMOVI32AsmOperand;
 }
 
 def nImmVMOVI16AsmOperandByteReplicate :
   AsmOperandClass {

 defm : NEONDTAnyInstAlias<"veor${p}", "$Vdn, $Vm",
                          (VEORd DPR:$Vdn, DPR:$Vdn, DPR:$Vm, pred:$p)>;
 defm : NEONDTAnyInstAlias<"veor${p}", "$Vdn, $Vm",
                          (VEORq QPR:$Vdn, QPR:$Vdn, QPR:$Vm, pred:$p)>;
 defm : NEONDTAnyInstAlias<"vorr${p}", "$Vdn, $Vm",
                          (VORRd DPR:$Vdn, DPR:$Vdn, DPR:$Vm, pred:$p)>;
 defm : NEONDTAnyInstAlias<"vorr${p}", "$Vdn, $Vm",
                          (VORRq QPR:$Vdn, QPR:$Vdn, QPR:$Vm, pred:$p)>;
+// ... immediates
+def : NEONInstAlias<"vand${p}.i16 $Vd, $imm",
+                    (VBICiv4i16 DPR:$Vd, nImmSplatNotI16:$imm, pred:$p)>;
+def : NEONInstAlias<"vand${p}.i32 $Vd, $imm",
+                    (VBICiv2i32 DPR:$Vd, nImmSplatNotI32:$imm, pred:$p)>;
+def : NEONInstAlias<"vand${p}.i16 $Vd, $imm",
+                    (VBICiv8i16 QPR:$Vd, nImmSplatNotI16:$imm, pred:$p)>;
+def : NEONInstAlias<"vand${p}.i32 $Vd, $imm",
+                    (VBICiv4i32 QPR:$Vd, nImmSplatNotI32:$imm, pred:$p)>;
+
 
 // VLD1 single-lane pseudo-instructions. These need special handling for
 // the lane index that an InstAlias can't handle, so we use these instead.
 def VLD1LNdAsm_8 : NEONDataTypeAsmPseudoInst<"vld1${p}", ".8", "$list, $addr",
                  (ins VecListOneDByteIndexed:$list, addrmode6alignNone:$addr,
                       pred:$p)>;
 def VLD1LNdAsm_16 : NEONDataTypeAsmPseudoInst<"vld1${p}", ".16", "$list, $addr",
                  (ins VecListOneDHWordIndexed:$list, addrmode6align16:$addr,

lib/Target/ARM/AsmParser/ARMAsmParser.cpp

     const MCConstantExpr *CE = dyn_cast<MCConstantExpr>(getImm());
     // Must be a constant.
     if (!CE) return false;
     int64_t Value = CE->getValue();
     // i16 value in the range [0,255] or [0x0100, 0xff00]
     return (Value >= 0 && Value < 256) || (Value >= 0x0100 && Value <= 0xff00);
   }
 
+  bool isNEONi16splatNot() const {
+    return isNEONi16splat();
+  }
+
   bool isNEONi32splat() const {
     if (isNEONByteReplicate(4))
       return false; // Leave that for bytes replication and forbid by default.
     if (!isImm())
       return false;
     const MCConstantExpr *CE = dyn_cast<MCConstantExpr>(getImm());
     // Must be a constant.
     if (!CE) return false;
     int64_t Value = CE->getValue();
     // i32 value with set bits only in one byte X000, 0X00, 00X0, or 000X.
     return (Value >= 0 && Value < 256) ||
       (Value >= 0x0100 && Value <= 0xff00) ||
       (Value >= 0x010000 && Value <= 0xff0000) ||
       (Value >= 0x01000000 && Value <= 0xff000000);
   }
 
+  bool isNEONi32splatNot() const {
+    return isNEONi32splat();
+  }
+
   bool isNEONByteReplicate(unsigned NumBytes) const {
     if (!isImm())
       return false;
     const MCConstantExpr *CE = dyn_cast<MCConstantExpr>(getImm());
     // Must be a constant.
     if (!CE)
       return false;
     int64_t Value = CE->getValue();
     if (!Value)
       return false; // Don't bother with zero.
 

   void addNEONi8splatOperands(MCInst &Inst, unsigned N) const {
     assert(N == 1 && "Invalid number of operands!");
     // The immediate encodes the type of constant as well as the value.
     // Mask in that this is an i8 splat.
     const MCConstantExpr *CE = dyn_cast<MCConstantExpr>(getImm());
     Inst.addOperand(MCOperand::CreateImm(CE->getValue() | 0xe00));
   }
 
+private:
+  unsigned validateNEONi16splatOperands(unsigned Value) const {
+    if (Value >= 256)
+      Value = (Value >> 8) | 0xa00;
+    else
+      Value |= 0x800;
+    return Value;
+  }
+  unsigned validateNEONi32splatOperands(unsigned Value) const {
+    if (Value >= 256 && Value <= 0xff00)
+      Value = (Value >> 8) | 0x200;
+    else if (Value > 0xffff && Value <= 0xff0000)
+      Value = (Value >> 16) | 0x400;
+    else if (Value > 0xffffff)
+      Value = (Value >> 24) | 0x600;
+    return Value;
+  }
+
+public:
   void addNEONi16splatOperands(MCInst &Inst, unsigned N) const {
     assert(N == 1 && "Invalid number of operands!");
     // The immediate encodes the type of constant as well as the value.
     const MCConstantExpr *CE = dyn_cast<MCConstantExpr>(getImm());
     unsigned Value = CE->getValue();
-    if (Value >= 256)
-      Value = (Value >> 8) | 0xa00;
-    else
-      Value |= 0x800;
+    Value = validateNEONi16splatOperands(Value);
+    Inst.addOperand(MCOperand::CreateImm(Value));
+  }
+  void addNEONi16splatNotOperands(MCInst &Inst, unsigned N) const {
+    assert(N == 1 && "Invalid number of operands!");
+    // The immediate encodes the type of constant as well as the value.
+    const MCConstantExpr *CE = dyn_cast<MCConstantExpr>(getImm());
+    unsigned Value = CE->getValue();
+    Value = ~Value & 0xffff;
+    Value = validateNEONi16splatOperands(Value);
     Inst.addOperand(MCOperand::CreateImm(Value));
   }
 
   void addNEONi32splatOperands(MCInst &Inst, unsigned N) const {
     assert(N == 1 && "Invalid number of operands!");
     // The immediate encodes the type of constant as well as the value.
     const MCConstantExpr *CE = dyn_cast<MCConstantExpr>(getImm());
     unsigned Value = CE->getValue();
-    if (Value >= 256 && Value <= 0xff00)
-      Value = (Value >> 8) | 0x200;
-    else if (Value > 0xffff && Value <= 0xff0000)
-      Value = (Value >> 16) | 0x400;
-    else if (Value > 0xffffff)
-      Value = (Value >> 24) | 0x600;
+    Value = validateNEONi32splatOperands(Value);
+    Inst.addOperand(MCOperand::CreateImm(Value));
+  }
+  void addNEONi32splatNotOperands(MCInst &Inst, unsigned N) const {
+    assert(N == 1 && "Invalid number of operands!");
+    // The immediate encodes the type of constant as well as the value.
+    const MCConstantExpr *CE = dyn_cast<MCConstantExpr>(getImm());
+    unsigned Value = CE->getValue();
+    Value = ~Value & 0xffffffff;
+    Value = validateNEONi32splatOperands(Value);
     Inst.addOperand(MCOperand::CreateImm(Value));
   }
 
   void addNEONinvByteReplicateOperands(MCInst &Inst, unsigned N) const {
     assert(N == 1 && "Invalid number of operands!");
     // The immediate encodes the type of constant as well as the value.
     const MCConstantExpr *CE = dyn_cast<MCConstantExpr>(getImm());
     unsigned Value = CE->getValue();

test/MC/ARM/neon-bitwise-encoding.s

 
 @ CHECK: vbic  d16, d17, d16           @ encoding: [0xb0,0x01,0x51,0xf2]
 @ CHECK: vbic  q8, q8, q9              @ encoding: [0xf2,0x01,0x50,0xf2]
 @ CHECK: vbic.i32  d16, #0xff000000 @ encoding: [0x3f,0x07,0xc7,0xf3]
 @ CHECK: vbic.i32  q8, #0xff000000 @ encoding: [0x7f,0x07,0xc7,0xf3]
 @ CHECK: vbic  q10, q10, q11           @ encoding: [0xf6,0x41,0x54,0xf2]
 @ CHECK: vbic  d9, d9, d1              @ encoding: [0x11,0x91,0x19,0xf2]
 
+  vand.i16 d10, #0x03ff
+  vand.i32 d10, #0x03ffffff
+  vand.i16 q10, #0x03ff
+  vand.i32 q10, #0x03ffffff
+
+@ CHECK: vbic.i16  d10, #0xfc00    @ encoding: [0x3c,0xab,0x87,0xf3]
+@ CHECK: vbic.i32  d10, #0xfc000000 @ encoding: [0x3c,0xa7,0x87,0xf3]
+@ CHECK: vbic.i16  q10, #0xfc00    @ encoding: [0x7c,0x4b,0xc7,0xf3]
+@ CHECK: vbic.i32  q10, #0xfc000000 @ encoding: [0x7c,0x47,0xc7,0xf3]
 
   vorn  d16, d17, d16
   vorn  q8, q8, q9
 
 @ CHECK: vorn  d16, d17, d16           @ encoding: [0xb0,0x01,0x71,0xf2]
 @ CHECK: vorn  q8, q8, q9              @ encoding: [0xf2,0x01,0x70,0xf2]
 
   vmvn  d16, d16