Page MenuHomePhabricator

Add support for AArch64 UDF instruction
ClosedPublic

Authored by dnsampaio on Oct 16 2018, 3:17 AM.

Details

Summary

UDF

Permanently Undefined generates an Undefined Instruction exception (ESR_ELx.EC = 0b000000).
The encodings for UDF used in this section are defined as permanently undefined in the ARMv8-A architecture.
https://developer.arm.com/products/architecture/cpu-architecture/a-profile/docs/ddi0596/a/a64-base-instructions-alphabetic-order/udf-permanently-undefined

Diff Detail

Repository
rL LLVM

Event Timeline

dnsampaio created this revision.Oct 16 2018, 3:17 AM
javed.absar added inline comments.Oct 16 2018, 3:47 AM
lib/Target/AArch64/AArch64InstrFormats.td
4088 ↗(On Diff #169803)

The 'uimm16' is generic and should be pulled up to where others (e.g. uimmm6) are defined.

test/MC/AArch64/udf.s
14 ↗(On Diff #169803)

Some of these look unnecessary (just bounaries and a value in between is enough)

dnsampaio marked an inline comment as done.Oct 16 2018, 4:29 AM

To send patch....

lib/Target/AArch64/AArch64InstrFormats.td
4088 ↗(On Diff #169803)

I can't if to reuse the existing ParserMatchClass = Imm0_65535Operand. I followed the same pattern of the // Move immediate., that defined the immediate types just before the instructions.

dnsampaio updated this revision to Diff 169809.Oct 16 2018, 4:50 AM

Fixed the tests as to check boundaries, and one value in the middle.
Changed tests ordering as they generated empty lines not treated by FileCheck.

nhaehnle added inline comments.Oct 16 2018, 5:07 AM
lib/Target/AArch64/AArch64InstrFormats.td
4088 ↗(On Diff #169803)

Note that at least the TableGen frontend allows you to inline expressions such as AsmImmRange<0, 65535> directly into the let statement.

The generated records will be de-duplicated, except in the case of an explicit instantiation (like the existing def Imm0_65535Operand).

I don't know if existing backends would stumble over a mixture of both explicit and implicit instantiations for ParserMatchClass, but consistently using inline AsmImmRange expressions everywhere should work.

(Just my 2c of TableGen info, do with it whatever you like, as I'm not working on AArch64 ;))

dnsampaio added inline comments.Oct 16 2018, 10:22 AM
lib/Target/AArch64/AArch64InstrFormats.td
4088 ↗(On Diff #169803)

I don't believe the de-duplication works very well then.
If I do change let ParserMatchClass = Imm0_65535Operand to let ParserMatchClass = AsmImmRange<0x0, 0xffff>; I get

In file included from /work/bf/LLVM/local/src/lib/Target/AArch64/AsmParser/AArch64AsmParser.cpp:5449:0:
lib/Target/AArch64/AArch64GenAsmMatcher.inc:7354:3: error: redefinition of 'MCK_Imm0_65535'
   MCK_Imm0_65535, // user defined class 'anonymous_1075'
   ^
lib/Target/AArch64/AArch64GenAsmMatcher.inc:7139:3: note: 'MCK_Imm0_65535' previously defined here
   MCK_Imm0_65535, // user defined class 'Imm0_65535Operand'
   ^
lib/Target/AArch64/AArch64GenAsmMatcher.inc: In function 'unsigned int validateOperandClass(llvm::MCParsedAsmOperand&, {anonymous}::MatchClassKind)':
lib/Target/AArch64/AArch64GenAsmMatcher.inc:10405:3: error: duplicate case value
   case MCK_Imm0_65535: {
   ^
lib/Target/AArch64/AArch64GenAsmMatcher.inc:8644:3: error: previously used here
   case MCK_Imm0_65535: {
   ^
lib/Target/AArch64/AArch64GenAsmMatcher.inc: In function 'const char* getMatchClassName({anonymous}::MatchClassKind)':
lib/Target/AArch64/AArch64GenAsmMatcher.inc:11852:3: error: duplicate case value
   case MCK_Imm0_65535: return "MCK_Imm0_65535";
   ^
lib/Target/AArch64/AArch64GenAsmMatcher.inc:11637:3: error: previously used here
   case MCK_Imm0_65535: return "MCK_Imm0_65535";
   ^
lib/Target/AArch64/AArch64GenAsmMatcher.inc:11445:10: warning: enumeration value 'MCK_Imm0_65535' not handled in switch [-Wswitch]
   switch (Kind) {

No custom decoding is required

nhaehnle added inline comments.Oct 16 2018, 10:59 AM
lib/Target/AArch64/AArch64InstrFormats.td
4088 ↗(On Diff #169803)

Yeah, that confirms my suspicion that things break when you mix explicit with implicit definitions. The redefinition comes from the fact that you still have a named def that is equivalent to the anonymous def that's generated by the inline mention.

dnsampaio updated this revision to Diff 170600.Oct 23 2018, 3:45 AM

Refactoring.

Could you add assembler/disassembler tests for udf ?

lib/Target/AArch64/AArch64InstrFormats.td
300 ↗(On Diff #170600)

maybe the formatting should be similar to surrounding code ( [{.*}] one same line).

dnsampaio marked an inline comment as done.Oct 29 2018, 4:22 AM
dnsampaio updated this revision to Diff 171483.Oct 29 2018, 4:25 AM

Re-inserted the missing tests. Small refactoring as to look alike neighbor code.

dnsampaio updated this revision to Diff 171657.Oct 30 2018, 3:35 AM

Added blank lines at end of test files.

javed.absar accepted this revision.Oct 30 2018, 3:39 AM
This revision is now accepted and ready to land.Oct 30 2018, 3:39 AM
This revision was automatically updated to reflect the committed changes.