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
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
