[PATCH] D119391: [X86] Only force FP usage in the presence of pushf/popf on Win64

Wed Feb 9 18:22:15 PST 2022

rnk added a comment.

In D119391#3309645 <https://reviews.llvm.org/D119391#3309645>, @nickdesaulniers wrote:

> This approach works, too (in addition to D92695 <https://reviews.llvm.org/D92695>) and is simpler (though, should `X86FrameLowering::has128ByteRedZone` now return `false` for frame using the intrinsics that aren't wincfi? If so, then modification to that method too will degenerate into D92695 <https://reviews.llvm.org/D92695>).

Yeah, my understanding of `has128ByteRedZone` is that it is meant to be a target check: does the target have a redzone by convention? So, I think it makes sense to keep the function-specific compatibility checks at the call site where it is used.

> Consider whether D119382 <https://reviews.llvm.org/D119382> still has value; if it does, perhaps it should land first and/or this patch made a child of it; otherwise happy to abandon it.

Now that I found the existing in tree x86-64-flags-intrinsics test, I think we could skip that new test.

> Also, in D92695 <https://reviews.llvm.org/D92695>, I modify llvm/test/CodeGen/X86/x86-64-flags-intrinsics.ll. I'm not sure what's preferred for that test. PTAL

You know I went looking for that, but did not find it... I will update it to have it check win64 and linux codegen, since those behave differently now.

> It would be helpful to have in the commit message for this patch that it
>
> Fixes: https://github.com/llvm/llvm-project/issues/46875

Sure.

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D119391