Page MenuHomePhabricator

[x86] Model the direction flag (DF) separately from the rest of EFLAGS.
ClosedPublic

Authored by chandlerc on Apr 1 2018, 4:20 PM.

Details

Summary

This cleans up a number of operations that only claimed te use EFLAGS
due to using DF. But no instructions which we think of us setting EFLAGS
actually modify DF (other than things like popf) and so this needlessly
creates uses of EFLAGS that aren't really there.

In fact, DF is so restrictive it is pretty easy to model. Only STD, CLD,
and the whole-flags writes (WRFLAGS and POPF) need to model this.

Sadly, this runs into issues because some part of the x86 backend
I can't find is marking uses of EFLAGS prior to a def as 'undef' very
helpfully. I need that to happen for DF as well. So far, no joy. If
anyone has suggestions, that'd be awesome and then we could land this
patch.

Diff Detail

Repository
rL LLVM

Event Timeline

chandlerc created this revision.Apr 1 2018, 4:20 PM

I need that to happen for DF as well.

You do...? DF isn't actually undefined at any point; the x86 ABI says DF is clear on entry to/exit from a function.

I need that to happen for DF as well.

You do...? DF isn't actually undefined at any point; the x86 ABI says DF is clear on entry to/exit from a function.

Sorry for being imprecise...

I need *some* magic for making the LLVM "use of undefined physreg" complaints go away. It could be marking it as defined on entry. It could be something else.

craig.topper added inline comments.Apr 2 2018, 10:01 PM
llvm/lib/Target/X86/X86InstrInfo.td
2120 ↗(On Diff #140601)

Don't these also use EFLAGS to pass through the other bits?

llvm/lib/Target/X86/X86InstrSystem.td
703 ↗(On Diff #140601)

This isn't a VMX instruction. It was introduced in the 286. Virtualization has to be aware of it to properly virtualize it.

chandlerc marked 2 inline comments as done.Apr 2 2018, 10:34 PM
chandlerc added inline comments.
llvm/lib/Target/X86/X86InstrSystem.td
703 ↗(On Diff #140601)

Sorry, I should have finished reading that sentence in the Intel manual rather than just seeing VMX and running with it. =/ Tried to provide more useful comment.

chandlerc marked an inline comment as done.Apr 2 2018, 10:35 PM
chandlerc updated this revision to Diff 140741.Apr 2 2018, 11:33 PM

Update fixing the remaining issues and rebasing on top of the EFLAGS patch.

With this, all my test failures are gone and I think this patch is actually ready-to-go.

chandlerc updated this revision to Diff 141780.Apr 9 2018, 6:51 PM

Rebase now that the core flags change has landed.

chandlerc updated this revision to Diff 141782.Apr 9 2018, 6:59 PM

Fix a remaining place where we need to mark EFLAGS as used as we pass through
some of the flags.

Also teach clang-format to be happy w/ some of the macro code in the disassembler.

craig.topper added inline comments.Apr 9 2018, 7:16 PM
llvm/lib/Target/X86/Disassembler/X86Disassembler.cpp
273 ↗(On Diff #141782)

Is this even needed? Was someone trying to protect against a trailing comma in the macro expansion? Which we now have outside...

llvm/lib/Target/X86/X86RegisterInfo.td
523 ↗(On Diff #141782)

I feel like CCR is a bad name here since that should mean "condition code register" and DF is not that.

chandlerc added inline comments.Apr 9 2018, 7:22 PM
llvm/lib/Target/X86/Disassembler/X86Disassembler.cpp
273 ↗(On Diff #141782)

I assumed this was trying to create a null terminator.

I thought we made trailing commas a no-op for these things so that stuff like this was OK?

llvm/lib/Target/X86/X86RegisterInfo.td
523 ↗(On Diff #141782)

Any better naming ideas?

craig.topper added inline comments.Apr 9 2018, 7:35 PM
llvm/lib/Target/X86/Disassembler/X86Disassembler.cpp
273 ↗(On Diff #141782)

Yeah trailing commas are fine for this reason, except for in enums pre-C++11. I've personally fixed a number of tablegen emitters to not pad arrays with a 0 purely to prevent trailing commas even though it should never have been an issue. So I can believe that this is there for that reason. Maybe remove it and check with an assert against array_lengthof.

Should that array be declared "static const"? Does that make much difference to the code we output?

chandlerc updated this revision to Diff 141790.Apr 9 2018, 10:29 PM

Clean up disassembler array a bit.

chandlerc marked 3 inline comments as done.Apr 9 2018, 10:30 PM
chandlerc added inline comments.
llvm/lib/Target/X86/Disassembler/X86Disassembler.cpp
273 ↗(On Diff #141782)

Hey, it works!

Removed all the cruft, directly expanded the macro in the curly braces.

I switched to static constexpr. Technically speaking with const it remains ambiguous whether the initializer will be run dynamically (which would be unfortunate, once-init barrier and everything). But with constexpr we get what we really want: an actual constant blob of data with nothing else going on.

craig.topper accepted this revision.Apr 9 2018, 10:47 PM

I don't have a better name for DFCCR. So I'm fine leaving it as is.

LGTM

This revision is now accepted and ready to land.Apr 9 2018, 10:47 PM
This revision was automatically updated to reflect the committed changes.
chandlerc marked an inline comment as done.