[PATCH] D42780: [RISCV] CompressPat Tablegen-driven Instruction Compression

[Alex Bradbury via Phabricator via llvm-commits]

asb added a comment.

Thanks for the test changes.

I think this is now looking good to land if my minor comments are addressed. As with any piece of code there may be further cleanups or improvements to make in the future, but they are probably best tackled once in-tree.

Do you think some of the uses of assert and llvm_unreachable in the tablegen emitter might be better handled as a fatal error to ensure we error out in non-debug builds? It's more developer friendly that way, and it's not uncommon for people to use -DLLVM_OPTIMIZED_TABLEGEN=True to improve build times, meaning that even in your debug-enabled LLVM build you get a non-debug Tablegen.

>From your perspective, is there any aspect in this patch that you'd like people to take a closer look at?



================
Comment at: include/llvm/MC/MCInst.h:117
+  bool evaluateAsSymbolRef() const;
+  bool evaluateAsConstantImm(int64_t &Imm) const;
   static MCOperand createReg(unsigned Reg) {
----------------
Newline after this?


================
Comment at: lib/MC/MCInst.cpp:47
+
+bool MCOperand::evaluateAsSymbolRef() const {
+  assert(isExpr() &&
----------------
Would this function be better named as isSymbolRef or isBareSymbolRef or something that reflects the fact it simply returns true if we have a SymbolRef with a MCSymbolRefExpr::VK_None kind.


================
Comment at: test/CodeGen/RISCV/compress-inline-asm.ll:20-25
+attributes #0 = { noinline nounwind optnone "correctly-rounded-divide-sqrt-fp-math"="false" "disable-tail-calls"="false" "less-precise-fpmad"="false" "no-frame-pointer-elim"="true" "no-frame-pointer-elim-non-leaf" "no-infs-fp-math"="false" "no-jump-tables"="false" "no-nans-fp-math"="false" "no-signed-zeros-fp-math"="false" "no-trapping-math"="false" "stack-protector-buffer-size"="8" "unsafe-fp-math"="false" "use-soft-float"="false" }
+attributes #1 = { nounwind }
+
+!llvm.module.flags = !{!0}
+!0 = !{i32 1, !"wchar_size", i32 4}
+!2 = !{i32 30}
----------------
I don't think we need this extraneous stuff from clang. I'd just keep this as a truly minimal test case, like one of the example in test/CodeGen/RISCV/inline-asm.ll.


https://reviews.llvm.org/D42780