[PATCH] D55506: [RFC v2] Allow target to handle STRICT floating-point nodes

Mon Dec 17 16:19:24 PST 2018

andrew.w.kaylor added a comment.

This looks like a promising direction. I particularly like the idea of having a way to intersect information from the backend instruction definitions with the constraints coming from the IR. However, I also have some concerns.

It seems that you're doing two things in this patch -- (1) adding the FCP register to the SystemZ backend, and (2) adding the strict FP handshake mechanism. Could you separate those two change sets?

I'd the new flags being added to make specific reference to FP. People who don't do a lot of FP-specific work are likely to misunderstand any unqualified term such as mayRaiseException. For the MIFlag I'd prefer something like FPStrict because I expect that we will want to extend this to handling rounding mode issues like constant folding or code motion relative to instructions that change the rounding mode.

The unmodeled side effects approach is a lot stronger than we really need. Ideally, these instructions would only act as a barrier to one another and not to other instructions. My concern is that simply marking them as having unmodeled side effects is going to have a bigger hit on optimizations than we want. Granted, the same thing is happening at the IR level with the intrinsics but I have a rough idea of how we'll be able to move forward in that case without rewriting the mechanism.

What I'd really like to see is a way for the backend to be able to completely model the relevant FP register uses/defs but conditionally strip those uses/defs out for non-strict instructions. I explored this with the X86 backend. I had to create an artificial split between the control and status parts of the MXCSR register. I ran into problems because every FP instruction had to be a use and a def of the status bits. That would be fine for strict mode, but it wasn't really acceptable as a default behavior. I only have a rough idea of how this would work.

Another problem I ran into when I tried to add modeling of the MXCSR register is that the machine verifier didn't like seeing uses of this register without ever having seen a def. That didn't show up in my initial testing and I got as far as committing something that modeled the use of MXCSR by the instructions that explicitly read and write it, but the machine verifier issue appeared in post-commit testing. I think it came up with inline assembly that was using the STMXCSR instruction directly. It seems like this could be an issue with the FCP register you're adding for System Z also.

Eventually I'd really like a way to pass the rounding mode argument through to the backend. I've been emphatic in the past that this argument is meant to describe the assumed rounding mode rather than control it, but it occurs to me that for architectures which have FP instructions where the rounding mode can/must be baked into the instruction the backend could use the **assumed** rounding mode to set the rounding mode operand. In X86 we've only got a few instructions like this, but it's my understanding that some other architectures require it more broadly.

================
Comment at: include/llvm/CodeGen/SelectionDAGNodes.h:469
   bool isFast() const {
-    return NoSignedZeros && AllowReciprocal && NoNaNs && NoInfs &&
+    return NoSignedZeros && AllowReciprocal && NoNaNs && NoInfs && NoExcept &&
            AllowContract && ApproximateFuncs && AllowReassociation;
----------------
There has been some discussion in the past about the possibility of using the fast math flags while still preserving strict FP exceptions. I'm not sure we reached a conclusion. 

================
Comment at: lib/CodeGen/SelectionDAG/SelectionDAGBuilder.cpp:6520
+  SDNodeFlags Flags;
+  Flags.setFPExcept(true);
+  Result->setFlags(Flags);
----------------
This really shouldn't be unconditional. It's possible to have constrained intrinsics that are preserving the rounding mode but ignoring FP exceptions. At this point we still have that information.

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

https://reviews.llvm.org/D55506