[PATCH] D102585: [M68k] Support inline asm operands w/ simple constraints

[Nick Desaulniers via Phabricator via llvm-commits]

nickdesaulniers added a comment.

Looking good @myhsu .  Also, I got your LLVM book recently!  You'll need to sign it for me at the next llvm conf in person.



================
Comment at: clang/lib/Basic/Targets/M68k.cpp:197-199
+    std::string R = std::string("^") + std::string(Constraint, 2);
+    ++Constraint;
+    return R;
----------------
    return std::string("^") + std::string(Constraint++, 2);


================
Comment at: clang/test/Sema/inline-asm-validate-m68k.c:10
+  asm volatile ("" :: "I"(BelowMin)); // expected-error{{value '0' out of range for constraint 'I'}}
+  asm volatile ("" :: "I"(AboveMax)); // expected-error{{value '9' out of range for constraint 'I'}}
+}
----------------
Tests look good. Would you mind please adding an `asm` for each function that doesn't require an `expected-error`? ie. is valid input?

Also, `volatile` keyword is not necessary when there are no output operands. See also: https://gcc.gnu.org/onlinedocs/gcc/Extended-Asm.html#Volatile.


================
Comment at: clang/test/Sema/inline-asm-validate-m68k.c:51
+  asm volatile ("" :: "C0"(IncorrectVal)); // expected-error{{value '1' out of range for constraint 'C0'}}
+}
----------------
do we need tests for lowercase `a`, `r`, or `d`?


================
Comment at: llvm/lib/Target/M68k/M68kAsmPrinter.cpp:57-59
+  case MachineOperand::MO_ConstantPoolIndex:
+    OS << DL.getPrivateGlobalPrefix() << "CPI" << getFunctionNumber() << '_'
+       << MO.getIndex();
----------------
If `DL` is only needed for this lone case, consider using `{}` for the case and sinking the def closer to the use.


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

https://reviews.llvm.org/D102585