[PATCH] D25618: Check that emitted instructions meet their predicates on all targets except ARM, Mips, and X86.

[Daniel Sanders via llvm-commits]

dsanders added a comment.

Thanks for the quick feedback.



================
Comment at: lib/Target/AArch64/MCTargetDesc/AArch64MCCodeEmitter.cpp:175
+private:
+  uint64_t ComputeAvailableFeatures(const FeatureBitset &FB) const;
+  void verifyInstructionPredicates(const MCInst &MI,
----------------
jmolloy wrote:
> lowerCamelCase
I agree it should be a lower case first letter. The reason it isn't is because the generated function is shared with the AsmMatcher and that one isn't a member function. I'll fix this.


================
Comment at: lib/Target/AMDGPU/VOP1Instructions.td:539
   let VOP1 = 1;
+  let SubtargetPredicate = isGCN;
 }
----------------
tstellarAMD wrote:
> dsanders wrote:
> > @tstellarAMD: Hi Tom, I'm not sure if this line is correct or not. I've included it because a recent change (r284031) left a '?' in this field and that causes problems in tablegen. Is there a correct predicate to use here?
> I think this needs to be isVI.
Thanks, I'll update the patch.


================
Comment at: utils/TableGen/CodeEmitterGen.cpp:347
+  o << "  assert((AvailableFeatures & RequiredFeatures[Inst.getOpcode()])\n"
+    << "             == RequiredFeatures[Inst.getOpcode()] &&\n"
+    << "         \"Attempting to emit an instruction that doesn't satisfy the required\"\n"
----------------
jmolloy wrote:
> This absolutely needs to print out the offending instruction, and ideally the required features although as they're encoded in a bitset that's probably quite difficult.
I agree, so far I've been re-running the failing command under lldb, printing Inst.getOpcode() and then looking that up in the generated AsmMatcher but that's a bit long winded. 

In the worst case I can generate a table but I'll see if I can re-use existing information first.



https://reviews.llvm.org/D25618