Page MenuHomePhabricator

[x86] avoid infinite loop from SoftenFloatOperand (PR34866)
ClosedPublic

Authored by spatel on Oct 10 2017, 3:38 PM.

Details

Summary

I don't have any knowledge of fp128 (why do we need -mattr=mmx?) or what's supposed to be happening in these tests, so I assumed we should just run away from this possibility.

Diff Detail

Repository
rL LLVM

Event Timeline

spatel created this revision.Oct 10 2017, 3:38 PM
RKSimon added inline comments.Oct 11 2017, 3:46 AM
lib/Target/X86/X86ISelLowering.cpp
35229 ↗(On Diff #118500)

Does this work?

if (peekThroughBitcasts(X).getValueType() == MVT::f128) ||
     peekThroughBitcasts(Y).getValueType() == MVT::f128))
spatel added inline comments.Oct 11 2017, 6:10 AM
lib/Target/X86/X86ISelLowering.cpp
35229 ↗(On Diff #118500)

Yes - that's better, thanks...and reminds me to chase down the remaining blockers for removing the loop in there.

spatel updated this revision to Diff 118601.Oct 11 2017, 6:21 AM

Patch updated:
Use peekThroughBitcasts() to make the code shorter and more robust.

Note:
I found this as the source of the MMX requirement, but I'm still not sure what is expected from these tests (other than not crashing):

// Long double always uses X87, except f128 in MMX.
if (UseX87) {
  if (Subtarget.is64Bit() && Subtarget.hasMMX()) {
    addRegisterClass(MVT::f128, &X86::FR128RegClass);
    ValueTypeActions.setTypeAction(MVT::f128, TypeSoftenFloat);

...for the no-mmx target, we would generate:

movq	%rsi, %xmm0
movq	%rdi, %xmm1
punpcklqdq	%xmm0, %xmm1    ## xmm1 = xmm1[0],xmm0[0]
pxor	%xmm0, %xmm0
pcmpeqb	%xmm1, %xmm0
pmovmskb	%xmm0, %eax
cmpl	$65535, %eax            ## imm = 0xFFFF
sete	%al
retq
chh edited edge metadata.Oct 11 2017, 10:22 AM

This change LGTM.
I added more comments in https://bugs.llvm.org/show_bug.cgi?id=34866.

Note that fp128-cast.ll tests only cover the cases with +mmx.
If the original change in D31156 could affect fp128 and no-mmx cases,
it would be better to add test cases for clang -mno-mmx and/or -mno-sse.

In D38771#894855, @chh wrote:

This change LGTM.
I added more comments in https://bugs.llvm.org/show_bug.cgi?id=34866.

Note that fp128-cast.ll tests only cover the cases with +mmx.
If the original change in D31156 could affect fp128 and no-mmx cases,
it would be better to add test cases for clang -mno-mmx and/or -mno-sse.

Thanks for the explanation. And yes, D31156 has no dependency on mmx. I'll add a no-mmx RUN for this file.

This revision was automatically updated to reflect the committed changes.