[PATCH] D69372: [X86][VARARG] Avoid spilling xmm vararg arguments.

[Reid Kleckner via Phabricator via llvm-commits]

rnk requested changes to this revision.
rnk added a comment.
This revision now requires changes to proceed.

> For the noimplicitfloat mode, the compiler mustn't generate
>  floating-point code if it was not asked directly to do so.

I don't think checking AL on function entry will work reliably. In general, musttail is meant to forward all possible register parameters. Normal arguments may be passed in XMM registers without setting AL. Consider this code:

  typedef float v4f32 __attribute__((__vector_size__(16), __aligned__(16)));
  int gv_i32;
  v4f32 gv_v4f32;
  void bar(int x, v4f32 v) {
    gv_i32 = x;
    gv_v4f32 = v;
  }
  void foo() {
    v4f32 v = {};
    bar(42, v);
  }

It should be possible to use musttail thunk with a prototype of `void (int, ...)` between the call to foo and bar. And, if that thunk contains a function call, it will need to spill&fill all XMM argument registers. If you look at the generated assembly for calling `bar`, AL is not set to 1:

  # %bb.0:                                # %entry
          pushq   %rax
          .cfi_def_cfa_offset 16
          xorps   %xmm0, %xmm0
          movl    $42, %edi
          callq   _Z3bariDv4_f

Is there actually a use case for musttail thunks and `noimplicitfloat`? Could we instead just declare that such a thunk represents explicit FP usage?

Furthermore, this will probably do the wrong thing on Windows, where AL is never set, to my knowledge. Consider this C++ code that uses musttail in the MS ABI:

  int gv_i32;
  double gv_f64;
  class Foo {
    void virtual bar(int x, double v);
    void foo();
  };
  void Foo::bar(int x, double v) {
    gv_i32 = x;
    gv_f64 = v;
  }
  void Foo::foo() {
    auto mp = &Foo::bar;
    (this->*mp)(42, 0.0);
  }

The virtual member pointer thunk needs to pass along XMM2, although it doesn't contain a function call.



================
Comment at: llvm/lib/Target/X86/X86ExpandPseudo.cpp:250
+
+    // skip CFI instructions
+    if (CurStackRestoreInstr->isCFIInstruction())
----------------
This looks like a lot of very fragile pattern matching. I would greatly prefer it if we didn't have to do this.


================
Comment at: llvm/lib/Target/X86/X86ISelLowering.cpp:3473
+        // VASTART_SAVE_XMM_REGS to avoid unneccessary spilling.
+        // See https://bugs.llvm.org/show_bug.cgi?id=42219.
+        MF.getRegInfo().addLiveIn(Reg);
----------------
We don't in general reference the issue tracker from code comments, only from test cases.


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

https://reviews.llvm.org/D69372