[PATCH] D47491: Expand constrained FP operations

[Cameron McInally via Phabricator via llvm-commits]

cameron.mcinally added inline comments.


================
Comment at: test/CodeGen/X86/vector-constrained-fp-intrinsics.ll:5
+; CHECK-LABEL: constrained_vector_pow
+; COMMON: pow
+define <2 x double> @constrained_vector_pow() {
----------------
andrew.w.kaylor wrote:
> cameron.mcinally wrote:
> > andrew.w.kaylor wrote:
> > > I think this check needs to do more than this. You should be verifying the complete expansion.
> > > 
> > > Also, can you add checks for some other instructions. Some of these won't require expansion, right?
> > > I think this check needs to do more than this. You should be verifying the complete expansion.
> > 
> > Thanks. I've added a check for the upper POW.
> > 
> > > Also, can you add checks for some other instructions. Some of these won't require expansion, right?
> > 
> > POW is the only strict operation that I've found that currently needs expansion on X86. That said, I have a full set of tests that cover each strict operation. I could add them if you'd like, but it's a bit superfluous for this change. I do intend to add the full set once other patches are sent upstream. Thought?
> I'd really like to see checks of the entire sequence to which this gets expanded. That seems like the only way to be certain it was expanded correctly. In some of the other tests I took a shortcut similar to what you did here because there was a 1-to-1 correspondence between the intrinsic and the instruction to which it was lowered. Even in that case it was probably not an ideal test.
> 
> Regarding instructions that aren't supposed to be expanded, what I would like to see here is a test that uses a vector form of the constrained intrinsic and a check that verifies that we generated the corresponding vector instruction.
Andrew, just to be clear, you'd like to check for more than the Op+MOVH? There's not a lot to the expansion with these. The only thing I left out of the checks is the loads to feed the POW. The rest is epilogue code.

I suspect you missed the change I made to check for the MOVH, but maybe I'm mistaken?

Or maybe you're suggesting a more complicated vector operation? One where we have to generate two scalar operations and then shuffle them back together? I could see some value in that.

%bb.0: # %entry
pushq	%rax
.cfi_def_cfa_offset 16
movsd	.LCPI0_0(%rip), %xmm0 # xmm0 = mem[0],zero
movsd	.LCPI0_1(%rip), %xmm1 # xmm1 = mem[0],zero
callq	pow
movlhps	%xmm0, %xmm0 # xmm0 = xmm0[0,0]
popq	%rax
.cfi_def_cfa_offset 8
retq


Repository:
  rL LLVM

https://reviews.llvm.org/D47491