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

Tue Jun 4 11:01:47 PDT 2019

uweigand added a comment.

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

> Under "fpexcept.strict" we cannot eliminate the compare because it may raise an exception (though we can still get rid of the JCC), but under "fpexcept.maytrap" we can eliminate the compare because our only promise is that we won't raise spurious exceptions.

Currently, it looks like the MI common optimizers cannot easily make that distinction; for example, the MI-level dead code elimination pass checks "isSafeToMove" to decide whether an instruction may be deleted.  Now, instructions that may raise FP exceptions are never safe to move even with fpexcept.maytrap, so that check must always fail.  If it ever becomes important to make that distinction, I'd add more fine-grained primitives like isSafeToMove vs. isSafeToDelete, and then add a second MI flag like NoDelete that would be set for fpexcept.strict insns.

As an aside, I'm not sure what this fpexcept.strict semantics is even necessary for; I don't believe it is required for the C standard FENV_ACCESS ON mode ...

> First, I'd like to avoid modeling the FP control and status registers when we aren't in a constrained mode. I haven't done any tests to measure the effect of modeling these registers, but it does place some constraints on the backend that don't need to be there in the unconstrained mode and at least has the potential to degrade performance.

As Craig pointed out, just adding a use cannot degrade performance.  I've already changed the SystemZ back-end to do this unconditionally, and it generated 100% identical assembler code as before (except in functions that use a built-in to modify the rounding mode, but there we actually want those changes).

> Second, there are some additional optimizations we could make in the cases where we know the rounding mode. Constant folding is an example. I'm not sure there are any backend optimizations that do constant folding after instruction selection, but there was a review recently where this exact issue came up for constant folding during ISel and the conclusion was that we could handle it if we knew the rounding mode but if not we'd have to just block the folding.

I do not believe it is ever possible to have constant folding at the MI level.  Common MI passes don't even understand whether an instruction is an add or a subtract (they only understand the general "form" of an instruction, e.g. what are input vs. output operands etc.); given that, it is hard to see where knowledge of a rounding mode could ever be useful at this level.  Those optimizations have to be done earlier.

> Third, there are some architectures where the rounding mode can be included as an operand. This is a potentially confusing point so I'm going to be verbose. I've said before that the rounding mode in constrained instructions is only a descriptive hint to the optimizer, declaring what the rounding mode is at that point rather than a  prescriptive operator that sets the rounding mode. However, in the case where we do know the rounding mode (i.e. the rounding mode operand is something other than "round.dynamic") we can use that information to select an instruction form that has a rounding mode operand. How important or unimportant this capability is will depend on the architecture, but in some cases I think it could be significant.

If platforms want to do that, my suggestion would be to make the rounding mode available during instruction selection (at the ISel DAG level) so that the platform can then select *different* MI instruction opcodes.  I think this also wouldn't conflict with anything in this current patch and can be added later.  There would be no need the to have the generic MI layer somehow encode rounding modes then.

> With regard to the patch as a whole, let me say that my biggest concern at this point is to make sure we aren't going toward a dead end. If we think that all of the issues I've raised can likely be addressed in some variation of the patch you've proposed here then I would be in favor of committing this patch now without solving all of those problems. I just don't want to put something in place that will need to be ripped out later in order to make progress. Of course I understand that there's a balance to be found here. I know some of what we have already will need to be replaced, and that's just the way things go. That is to say, I just want to make sure we're headed in more or less the right direction.
> 
> Thanks for your patience and persistence.

Thanks for the continued review!

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

https://reviews.llvm.org/D55506