[X86] Make `prefetchit{0/1}` emit an assembler warning if the operand is not rip-rel

[goldstein.w.n]

Summary

Without a rip-rel operand, prefetchit{0/1} is a nop. This is a
reasonable mistake for someone to make and is almost certainly not
what they are after.

This matches the same warning in gas.

[pengfei]

llvm/test/CodeGen/X86/prefetchi-warning.ll

1 ↗

(On Diff #492969)

This seems miss a half use case. User may use gas rather than native assembler. Maybe move it to early pass? X86InstrInfo::verifyInstruction looks like an ideal place, but it is just for error check.

[goldstein.w.n]

llvm/test/CodeGen/X86/prefetchi-warning.ll

1 ↗

(On Diff #492969)

This seems miss a half use case. User may use gas rather than native assembler. Maybe move it to early pass? X86InstrInfo::verifyInstruction looks like an ideal place, but it is just for error check.

Or pass files assembler by GCC/hand-written to llvm-as...
Imo it makes more sense in gas + llvm-as than earlier.

[goldstein.w.n]

Move tests to MC.
Make warning match verbatim with gas

[goldstein.w.n]

llvm/test/CodeGen/X86/prefetchi-warning.ll

1 ↗

(On Diff #492969)

This seems miss a half use case. User may use gas rather than native assembler. Maybe move it to early pass? X86InstrInfo::verifyInstruction looks like an ideal place, but it is just for error check.

So the warning exists in GAS at the moment, since these commits:

commit ef07be453e0edf2f43034fcbc0581f61e630993e
Author: Cui, Lili <[e-mail]>
Date:   Mon Oct 31 21:07:17 2022 +0800

    Support Intel PREFETCHI

commit f2462532e24ebfc137598d73ee6541948121f040
Author: H.J. Lu <[e-mail]>
Date:   Mon Oct 31 09:01:15 2022 -0700

    x86: Silence GCC 12 warning on tc-i386.c

New warning matches verbatim with GAS.

[goldstein.w.n]

Fix commit msg

[pengfei]

LGTM.

[craig.topper]

Why not do this in X86AsmParser::validateInstruction?

[goldstein.w.n]

In D142797#4088581, @craig.topper wrote:

Why not do this in X86AsmParser::validateInstruction?

AFAICT it only throws compilation errors.

[craig.topper]

In D142797#4088584, @goldstein.w.n wrote:

In D142797#4088581, @craig.topper wrote:

Why not do this in X86AsmParser::validateInstruction?

AFAICT it only throws compilation errors.

I believe the uses of Warning in validateInstruction actually return false so they don't stop the parsing.

[goldstein.w.n]

In D142797#4088587, @craig.topper wrote:

In D142797#4088584, @goldstein.w.n wrote:

In D142797#4088581, @craig.topper wrote:

Why not do this in X86AsmParser::validateInstruction?

AFAICT it only throws compilation errors.

I believe the uses of Warning in validateInstruction actually return false so they don't stop the parsing.

Oh, I'm sorry I misread your earlier comment and thought you where referring to X86InstrInfo::verifyInstruction.

Yeah X86AsmParser::validateInstruction seems to make sense, will give it a shot for V3.

[goldstein.w.n]

Use validateInstruction for validating prefetchit0 + make tests a bit better

[goldstein.w.n]

In D142797#4088581, @craig.topper wrote:

Why not do this in X86AsmParser::validateInstruction?

Done.

[pengfei]

llvm/lib/Target/X86/AsmParser/X86AsmParser.cpp

3927–3928

How about Inst.getOperand(X86::AddrBaseReg)?

[goldstein.w.n]

Use X86::AddrBaseReg instead of looping

llvm/lib/Target/X86/AsmParser/X86AsmParser.cpp

if ((TSFlags & X86II::EncodingMask) == 0) {
     if (UsesRex && HReg != X86::NoRegister) {
       StringRef RegName = X86IntelInstPrinter::getRegisterName(HReg);
       return Error(Ops[0]->getStartLoc(),
                    "can't encode '" + RegName + "' in an instruction requiring "
                    "REX prefix");
     }
   }
 
+  if ((Opcode == X86::PREFETCHIT0 || Opcode == X86::PREFETCHIT1)) {
+    const MCOperand &MO = Inst.getOperand(X86::AddrBaseReg);
+    if (!MO.isReg() || MO.getReg() != X86::RIP)
+      return Warning(
+          Ops[0]->getStartLoc(),
+          Twine((Inst.getOpcode() == X86::PREFETCHIT0 ? "'prefetchit0'"
+                                                      : "'prefetchit1'")) +
+              " only supports RIP-relative address");
+  }
   return false;
 }
 
 static const char *getSubtargetFeatureName(uint64_t Val);
 
 void X86AsmParser::emitWarningForSpecialLVIInstruction(SMLoc Loc) {
   Warning(Loc, "Instruction may be vulnerable to LVI and "
                "requires manual mitigation");

llvm/test/MC/X86/prefetchit-non-rip-op.s

+// RUN: llvm-mc -triple x86_64-unknown-unknown %s > %t 2> %t.err
+// RUN: FileCheck --check-prefix=CHECK-STDERR < %t.err %s
+// RUN: FileCheck < %t %s
+
+// CHECK: prefetchit0 (%rdi)
+// CHECK-STDERR: warning: 'prefetchit0' only supports RIP-relative address
+          prefetchit0 (%rdi)
+
+// CHECK: prefetchit1 (%rcx)
+// CHECK-STDERR: warning: 'prefetchit1' only supports RIP-relative address
+          prefetchit1 (%rcx)
+
+// CHECK: prefetchit0 1(%rdx)
+// CHECK-STDERR: warning: 'prefetchit0' only supports RIP-relative address
+          prefetchit0 1(%rdx)
+
+// CHECK: prefetchit1 12(%rsi)
+// CHECK-STDERR: warning: 'prefetchit1' only supports RIP-relative address
+          prefetchit1 12(%rsi)
+
+// CHECK: prefetchit0 123(%r8,%rax)
+// CHECK-STDERR: warning: 'prefetchit0' only supports RIP-relative address
+          prefetchit0 123(%r8,%rax)
+
+// CHECK: prefetchit1 1234(%r9,%r10)
+// CHECK-STDERR: warning: 'prefetchit1' only supports RIP-relative address
+          prefetchit1 1234(%r9,%r10)
+
+// CHECK: prefetchit0 (%r11,%r12)
+// CHECK-STDERR: warning: 'prefetchit0' only supports RIP-relative address
+          prefetchit0 (%r11,%r12)
+
+// CHECK: prefetchit1 (%r13,%r14)
+// CHECK-STDERR: warning: 'prefetchit1' only supports RIP-relative address
+          prefetchit1 (%r13,%r14)
+
+// CHECK: prefetchit0 987(%rsp,%r15,4)
+// CHECK-STDERR: warning: 'prefetchit0' only supports RIP-relative address
+          prefetchit0 987(%rsp,%r15,4)
+
+// CHECK: prefetchit1 -1(%rbp,%rdi,8)
+// CHECK-STDERR: warning: 'prefetchit1' only supports RIP-relative address
+          prefetchit1 -1(%rbp,%rdi,8)
+
+// CHECK: prefetchit0 (%rsp,%rsi,2)
+// CHECK-STDERR: warning: 'prefetchit0' only supports RIP-relative address
+          prefetchit0 (%rsp,%rsi,2)
+
+// CHECK: prefetchit1 (%rdi,%r15,4)
+// CHECK-STDERR: warning: 'prefetchit1' only supports RIP-relative address
+          prefetchit1 (%rdi,%r15,4)
+
+// CHECK: prefetchit0 80(,%r14,8)
+// CHECK-STDERR: warning: 'prefetchit0' only supports RIP-relative address
+          prefetchit0 80(,%r14,8)
+
+// CHECK: prefetchit1 3(,%r8,4)
+// CHECK-STDERR: warning: 'prefetchit1' only supports RIP-relative address
+          prefetchit1 3(,%r8,4)
+
+// CHECK: prefetchit0 (,%rax,2)
+// CHECK-STDERR: warning: 'prefetchit0' only supports RIP-relative address
+          prefetchit0 (,%rax,2)
+
+// CHECK: prefetchit1 (,%rcx,8)
+// CHECK-STDERR: warning: 'prefetchit1' only supports RIP-relative address
+          prefetchit1 (,%rcx,8)