[PATCH] D151863: [x86][MC] Fix movdir64b addressing

[Fangrui Song via Phabricator via cfe-commits]

MaskRay added inline comments.


================
Comment at: llvm/test/CodeGen/X86/movdir64b-inline-asm-x86_64.ll:1
+; NOTE: Assertions have been autogenerated by utils/update_llc_test_checks.py UTC_ARGS: --version 2
+; RUN: llc < %s -mtriple=x86_64-unknown-unknown -mattr=+movdir64b | FileCheck %s --check-prefix=X64
----------------
akshaykhadse wrote:
> MaskRay wrote:
> > I think we should reuse an existing `*-inline-asm-*` test file.
> > 
> > See https://maskray.me/blog/2021-08-08-toolchain-testing#the-test-checks-at-the-wrong-layer "I don't know an existing test can be enhanced"
> **I could not find an appropriate existing test.**
> I added a couple of cases in the CFE tests `clang/test/CodeGen/ms-inline-asm-64.c` and `clang/test/CodeGen/ms-inline-asm.c`.
> Now, when I had to add backend tests, I searched for an appropriate file:
> ```
> llvm-project$ find llvm/test/CodeGen/X86 -type f -name ms-inline-asm-*
> llvm/test/CodeGen/X86/ms-inline-asm-variables-x86-2-regs.ll
> llvm/test/CodeGen/X86/ms-inline-asm-array.ll
> llvm/test/CodeGen/X86/ms-inline-asm-PR44272.ll
> llvm/test/CodeGen/X86/ms-inline-asm-variables-x64-2-regs.ll
> llvm/test/CodeGen/X86/ms-inline-asm-functions.ll
> llvm/test/CodeGen/X86/ms-inline-asm-redundant-clobber.ll
> llvm/test/CodeGen/X86/ms-inline-asm-variables-x64-1-reg.ll
> llvm/test/CodeGen/X86/ms-inline-asm-variables-x86-1-reg.ll
> llvm/test/CodeGen/X86/ms-inline-asm-avx512.ll
> llvm/test/CodeGen/X86/ms-inline-asm-variables-x64-nopic.ll
> ```
> As you can see, there are tests for variables, functions, arrays, but no general tests like `ms-inline-asm.ll`.
> My general observation is that for a CFE test `*-inline-asm*.c` there's an equivalent test in LLVM backend which mentions the original test name in the comment. For example, `ms-inline-asm-variables-x86-2-regs.ll` mentions that `Tests come from "clang/test/CodeGen/ms-inline-asm-variables.c"`.
> However, there is no such file which mentions either `ms-inline-asm.c` or `ms-inline-asm-64.c`
> So, I picked up an assembly instruction `xgetbv` from `ms-inline-asm.c` and searched it in the available `.ll` backend test via `llvm-project$ find llvm/test -type f -name *.ll -exec grep -l "xgetbv" {} \;`. There was just one file `llvm/test/CodeGen/X86/system-intrinsics-xgetbv.ll` that contains test for just this one instruction.
> **Please suggest an appropriate 32-bit and 64-bit file to add this test**
Thanks for the investigation. Sometimes we can consider refactoring existing tests before adding a new test. 

Even if we add a new file, `inline-asm-*.ll` may be a better name than `*-inline-asm.ll`, since the former allows code completion to find related tests more easily.

I'll run away:) My comment is to prompt investigation but I don't plan to look at the problem closely.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D151863