[X86] Permit reading of the FLAGS register without it being previously defined

[majnemer]

Summary

We modeled the RDFLAGS{32,64} operations as "using" {E,R}FLAGS.
While technically correct, this is not be desirable for folks who want
to examine aspects of the FLAGS register which are not related to
computation like whether or not CPUID is a valid instruction.

[MatzeB]

LGTM.

[qcolombet]

Hi David,

Looks to me like you have two different commits here:

• The modifications of the read flags builtin
• The undef of flags for push/pop instruction

Cheers,
Q.

lib/Target/X86/X86InstrInfo.td

1130 ↗

(On Diff #49550)

It feels wrong to me that the builtin that reads the flags does *not* use the flags.

[qcolombet]

Note: I am fine with the push/pop undef thing. But for the read flag change, I believe it would be better to mark eflags as live-in of the function.

[rnk]

In D17782#365909, @qcolombet wrote:

Hi David,

Looks to me like you have two different commits here:

• The modifications of the read flags builtin
• The undef of flags for push/pop instruction

Both changes are required to mollify the MI verifier, and they cannot be committed separately with tests.

lib/Target/X86/X86InstrInfo.td

1130 ↗

(On Diff #49550)

It does not use the flags in any sense that the register allocator should care about. We don't, and should not, support reading comparison flags produced by x86 arithmetic operations that we've selected.

This intrinsic exists to read external processor state in flags, such as the trap flag, interrupt flag, and direction flag, none of which are modeled by the backend.

I guess it would be good comment on that here, though. :)

[qcolombet]

This intrinsic exists to read external processor state in flags, such as the trap flag, interrupt flag, and direction flag, none of which are modeled by the backend.

I guess it would be good comment on that here, though. :)

+1 :).

With that, LGTM.

Cheers,
-Quentin

[MatzeB]

In D17782#365923, @qcolombet wrote:

Note: I am fine with the push/pop undef thing. But for the read flag change, I believe it would be better to mark eflags as live-in of the function.

To be honest, the whole builtin feels wrong to me. When you use the builtin you probably intend to read eflags produced by another instruction. I don't see how you would get the guarantee that the scheduler doesn't put stuff in between... Part of the problem that we read "whatever" here, is because the builtin works like that... (Though this is not a vote to take the builtin out given that it just replaces similarily hacky inline assembly or emulating as bad builtins in other compilers).

llvm/trunk/lib/Target/X86/X86ISelLowering.cpp

X86TargetLowering::EmitInstrWithCustomInserter(MachineInstr *MI,
   case X86::RDFLAGS32:
   case X86::RDFLAGS64: {
     DebugLoc DL = MI->getDebugLoc();
     const TargetInstrInfo *TII = Subtarget.getInstrInfo();
     unsigned PushF =
         MI->getOpcode() == X86::RDFLAGS32 ? X86::PUSHF32 : X86::PUSHF64;
     unsigned Pop =
         MI->getOpcode() == X86::RDFLAGS32 ? X86::POP32r : X86::POP64r;
-    BuildMI(*BB, MI, DL, TII->get(PushF));
+    MachineInstr *Push = BuildMI(*BB, MI, DL, TII->get(PushF));
+    // Permit reads of the FLAGS register without it being defined.
+    // This intrinsic exists to read external processor state in flags, such as
+    // the trap flag, interrupt flag, and direction flag, none of which are
+    // modeled by the backend.
+    Push->getOperand(2).setIsUndef();
     BuildMI(*BB, MI, DL, TII->get(Pop), MI->getOperand(0).getReg());
 
     MI->eraseFromParent(); // The pseudo is gone now.
     return BB;
   }
 
   case X86::WRFLAGS32:
   case X86::WRFLAGS64: {

llvm/trunk/lib/Target/X86/X86InstrInfo.td

 def PUSH32rmm: I<0xFF, MRM6m, (outs), (ins i32mem:$src), "push{l}\t$src",[],
                  IIC_PUSH_MEM>, OpSize32, Requires<[Not64BitMode]>;
 } // mayLoad, mayStore, SchedRW
 
 }
 
 let mayLoad = 1, mayStore = 1, usesCustomInserter = 1,
     SchedRW = [WriteRMW], Defs = [ESP] in {
-  let Uses = [ESP, EFLAGS] in
+  let Uses = [ESP] in
   def RDFLAGS32 : PseudoI<(outs GR32:$dst), (ins),
                    [(set GR32:$dst, (int_x86_flags_read_u32))]>,
                 Requires<[Not64BitMode]>;
 
-  let Uses = [RSP, EFLAGS] in
+  let Uses = [RSP] in
   def RDFLAGS64 : PseudoI<(outs GR64:$dst), (ins),
                    [(set GR64:$dst, (int_x86_flags_read_u64))]>,
                 Requires<[In64BitMode]>;
 }
 
 let mayLoad = 1, mayStore = 1, usesCustomInserter = 1,
     SchedRW = [WriteRMW] in {
   let Defs = [ESP, EFLAGS], Uses = [ESP] in

llvm/trunk/test/CodeGen/X86/x86-64-flags-intrinsics.ll

-; RUN: llc < %s | FileCheck %s
+; RUN: llc -verify-machineinstrs < %s | FileCheck %s
 target triple = "x86_64-pc-win32"
 
 declare i64 @llvm.x86.flags.read.u64()
 declare void @llvm.x86.flags.write.u64(i64)
 
 define i64 @read_flags() {
 entry:
   %flags = call i64 @llvm.x86.flags.read.u64()

llvm/trunk/test/CodeGen/X86/x86-flags-intrinsics.ll

-; RUN: llc < %s | FileCheck %s
+; RUN: llc -verify-machineinstrs < %s | FileCheck %s
 target triple = "i686-pc-win32"
 
 declare i32 @llvm.x86.flags.read.u32()
 declare void @llvm.x86.flags.write.u32(i32)
 
 define i32 @read_flags() {
 entry:
   %flags = call i32 @llvm.x86.flags.read.u32()