This is an archive of the discontinued LLVM Phabricator instance.

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

Authored by majnemer on Mar 1 2016, 2:19 PM.

Details

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.

Diff Detail

Event Timeline

majnemer updated this revision to Diff 49550.Mar 1 2016, 2:19 PM
majnemer retitled this revision from to [X86] Permit reading of the FLAGS register without it being previously defined.
majnemer updated this object.
majnemer added reviewers: rnk, MatzeB.
majnemer added a subscriber: llvm-commits.
MatzeB accepted this revision.Mar 1 2016, 2:24 PM
MatzeB edited edge metadata.

LGTM.

This revision is now accepted and ready to land.Mar 1 2016, 2:24 PM

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

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

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 edited edge metadata.Mar 1 2016, 4:31 PM

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

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 accepted this revision.Mar 1 2016, 4:34 PM
qcolombet added a reviewer: 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 added a comment.Mar 1 2016, 4:35 PM

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).

This revision was automatically updated to reflect the committed changes.