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

Tue Dec 18 00:53:17 PST 2018

uweigand added a comment.

In D55506#1333753 <https://reviews.llvm.org/D55506#1333753>, @andrew.w.kaylor wrote:

> 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.

Thanks for the review!

> 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?

Of course.  I'm keeping them as separate patches internally anyway, and was planning to commit them separately.  I just wanted to show how the full picture would look like in the end.

> 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.

I'd be fine with using mayRaiseFPException instead.  I specifically did **not** want to mix other aspects (like rounding modes) into it, but rather separate the concerns here.  See my previous comments to Cameron about rounding modes in general.  But if we do need to track those at the MI level, I'd rather prefer to add **another** bit in MIFlags, and keep the one I add here strictly about exceptions.

Code motion relative to instructions that change the rounding mode is already handled in my patch, via the dependency on FPC that is added to all FP instructions.

> 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.

Agreed.  In fact, that's really the main point why I want to have the MI flag specifically about **exceptions**, so that common code can be written to handle the flag just exactly as is required for FP exceptions and nothing else.  I just didn't want to complicate this initial patch, and therefore added only a quick (overly conservative, but correct) check to hasUnmodeledSideEffect.  Once this is in, we can refine the handling as a follow-on patch.  (For example, handle exactly the necessary dependencies for FP exceptions in ScheduleDAGInstrs::buildSchedGraph, which might e.g. allow two FP instructions to be swapped as long as no speculative execution of FP instructions is introduced.)

> 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.

That's why I was going away from using registers to model exceptions.  As you suggest, I'm also in effect performing a split between the control and status parts of the FPC register: I'm using the register at the MI level to model the control part, and using the mayRaiseException flag to model the status part.  The side effect implied by the flag includes both the trap and setting of the status bits.  This seems more straightforward than adding register defs, in particular since the latter overly constrain scheduling.

> 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.

I'm not really sure I see where this can help.  On SystemZ there are a few instructions that encode rounding modes, but those are mapped to separate DAG opcodes.  For example, frint / fnearbyint / ffloor / fceil / ftrunc / fround all map to the same SystemZ instruction, just with a different encoded rounding mode value.  The existing DAG instruction selection mechanism seems fine for that.

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

https://reviews.llvm.org/D55506