[PATCH] D54649: [FPEnv] Rough out constrained FCmp intrinsics

[Cameron McInally via Phabricator via llvm-commits]

cameron.mcinally marked an inline comment as done.
cameron.mcinally added inline comments.


================
Comment at: lib/Target/X86/X86ISelLowering.cpp:26106
   case ISD::SETCC:              return LowerSETCC(Op, DAG);
+  case ISD::STRICT_FSETCC:      return LowerSTRICT_FSETCC(Op, DAG);
   case ISD::SETCCCARRY:         return LowerSETCCCARRY(Op, DAG);
----------------
uweigand wrote:
> cameron.mcinally wrote:
> > uweigand wrote:
> > > cameron.mcinally wrote:
> > > > uweigand wrote:
> > > > > Why do you have this here?   It seems that this can never be called right now (at least without something like my D45576, right?)
> > > > It actually will be called. Some SETCCs are always custom lowered on x86, even for scalars. 
> > > > 
> > > > The other constrained vector operations are all legal or expanded to legal scalar operations. SETCC is the first operation that needs the special lowering.
> > > > 
> > > > Just to be pedantic, I do realize this code is punting right now. The TableGen changes in this patch are quite involved. So I'd like for us to focus on those first. I will handle the STRICT_FSETCC peculiarities in a future patch.
> > > But still, I don't see any place where common code would even call the back-end's getOperationAction on a STRICT_FSETCC.
> > > 
> > > E.g. when calling getStrictFPOperationAction with a STRICT_FSETCC, it will actually call the back-end with SETCC (not STRICT_FSETCC).  So this should already return Custom for you ...
> > > 
> > >But still, I don't see any place where common code would even call the back-end's getOperationAction on a STRICT_FSETCC.
> > 
> > I agree with this statement. We call getStrictFPOperationAction(...) which will return Custom from SETCC (for the STRICT_FSETCC node).
> > 
> > > E.g. when calling getStrictFPOperationAction with a STRICT_FSETCC, it will actually call the back-end with SETCC (not STRICT_FSETCC). So this should already return Custom for you ...
> > 
> > I also agree with this, but when the backend returns Custom for SETCC, we have to then Custom lower the STRICT_FSETCC node. We do not mutate the STRICT_FSETCC node into SETCC node in LegalizeOp(), where we call getStrictFPOperationAction().
> > 
> > I'm not sure I fully understand your point though (although I may admittedly be making a fundamental error). If you comment out this line in the code, you'll see the backtrace leading up to the problem.
> > 
> Ah OK, now I see it.
> 
> I guess what confused me most is that you added the setOperationAction statement for STRICT_FSETCC further up.  **That** statement really is the one that doesn't seem to have any effect.
> 
> **This** statement does have effect, it's just a bit weird that the reason why LowerOperation is called with a STRICT_FSETCC node is that setOperationAction for SETCC (not STRICT_FSETCC!) is Custom.
> 
> Taking a step back, I think the original thought behind the getStrictFPOperationAction logic was that if the target doesn't do anything special, all strict FP operations will just transparently work as if they were regular FP operations.  But that is not (any longer?) the case if the regular FP operation is marked as Custom: there will have to be target support, or else it doesn't work at all.
> 
> The question is, is it worth trying to preseve that original thought?  I guess one way to do that would be for common code to handle STRICT_ nodes where the corresponding regular node is Custom by mutating the node before calling LowerOperation on the regular node.   (Later, when we add support for the target to actually handle STRICT_ nodes directly, we'd first check whether the STRICT_ node itself is marked Custom, and then pass it directly to the back-end.)
> 
> I guess what confused me most is that you added the setOperationAction statement for STRICT_FSETCC further up. That statement really is the one that doesn't seem to have any effect.

You're right. I'll correct it now. Sorry about that.

> Taking a step back, I think the original thought behind the getStrictFPOperationAction logic was that if the target doesn't do anything special, all strict FP operations will just transparently work as if they were regular FP operations. But that is not (any longer?) the case if the regular FP operation is marked as Custom: there will have to be target support, or else it doesn't work at all.

Yes. This is the first StrictFP node that always needs to be Custom lowered. The others were general enough to not cause any problems.

> The question is, is it worth trying to preseve that original thought? I guess one way to do that would be for common code to handle STRICT_ nodes where the corresponding regular node is Custom by mutating the node before calling LowerOperation on the regular node. (Later, when we add support for the target to actually handle STRICT_ nodes directly, we'd first check whether the STRICT_ node itself is marked Custom, and then pass it directly to the back-end.)

That was what I originally did with the patch, but then changed it to this. :D

I don't know if there's a huge difference between the two implementations. Both are weird (and really not right). They mutate the StrictFP node too soon.

In any case, this problem originated from not having backend support for strict nodes in place. So either solution is temporary. When we have backend support for strict nodes, this code will go away.


Repository:
  rL LLVM

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

https://reviews.llvm.org/D54649