Page MenuHomePhabricator

Inform AST's UnaryOperator of FENV_ACCESS
Needs ReviewPublic

Authored by kpn on Oct 3 2018, 11:43 AM.

Details

Summary

The UnaryOperator needs to know about #pragma FENV_ACCESS to eventually properly emit the constrained math intrinsics. This diff just gets the information about the pragma into that part of the AST.

The visible behavior of clang is unaffected and so this cannot yet be tested.

Diff Detail

Event Timeline

kpn created this revision.Oct 3 2018, 11:43 AM

Maybe take this very slightly further so it becomes testable: change the expression evaluator (lib/AST/ExprConstant.cpp) so it treats floating-point operations as non-constant if they're potentially-overflowing.

You also need to update lib/Serialization/AST{Reader,Writer}Stmt.cpp to round-trip this information through AST files.

How do you intend for this to work in template instantiation? Do we record the pragmas in the AST (and likewise for the state on entry to each function) so that we can rebuild the FPOptions during instantiation, or should we be inheriting the FPOptions from the expression in the template onto the expression in the instantiation? In the latter case, it would make more sense to me to track FPOptions on the Scope rather than as a Sema member, so that the state is more-obviously unavailable during template instantiation.

include/clang/AST/Expr.h
1798–1800

Move this adjacent to the other bitfields so we don't spend 32 / 64 bits on it.

kpn updated this revision to Diff 169266.Oct 11 2018, 12:02 PM

Address review comments.

Add a test case. The test depends on D53157, but it reads easily enough. Richard, does this work for you?

lebedev.ri added inline comments.
include/clang/AST/Expr.h
1791

Why as the first one, and not after CanOverflow ...

1808

.. then you should be able to init it in ctor initializer, not in the body.

kpn added a comment.Oct 11 2018, 12:36 PM

I forgot to address the template question. I've spend the past 18 years doing SystemZ backend work on multiple {,JIT} compilers. I'm not really in a position to answer your question about how to handle getting these FP bits down into C++ templates. I hope that won't hold up getting FENV_ACCESS working for C compilations.

kpn updated this revision to Diff 169463.Oct 12 2018, 10:58 AM
kpn marked 2 inline comments as done.

Address review comments.

I think you have uploaded the wrong patch here.

kpn updated this revision to Diff 169464.Oct 12 2018, 11:11 AM

Upload the correct diff this time. Sorry about the confusion!

rsmith added a comment.EditedOct 12 2018, 12:01 PM
In D52839#1262449, @kpn wrote:

I forgot to address the template question. I've spend the past 18 years doing SystemZ backend work on multiple {,JIT} compilers. I'm not really in a position to answer your question about how to handle getting these FP bits down into C++ templates. I hope that won't hold up getting FENV_ACCESS working for C compilations.

Actually, I think it should. If your approach doesn't extend to C++, then you have the wrong approach, and we should redesign it now rather than spend more time on it. (Also, we use TreeTransforms in C sometimes, too, so this actually isn't even a C++-specific design problem.)

I think there are two possible approaches to take here:

  1. Treat the FP environment flags as a part of the global Sema state. Whenever Clang switches its current context, figure out the right FP environment for the new context, and switch the FP environment too. This'd necessitate storing the FP environment data on DeclContexts, which would likely otherwise be unnecessary.
  2. Track the FP environment somewhere more transient, probably on the Scope object. When parsing an operator, store the environment on the expression, and when performing a TreeTransform, inherit the state onto the new node.

I think option 2 seems cleaner.

kpn added a comment.Jan 25 2019, 9:39 AM
  1. Track the FP environment somewhere more transient, probably on the Scope object. When parsing an operator, store the environment on the expression, and when performing a TreeTransform, inherit the state onto the new node.

    I think option 2 seems cleaner.

In D53157 it looks like an API where the IRBuilder has a constrained FP switch is preferred. This way calls to the IRBuilder can remain as one liners.

Going that route, would it make more sense to store the FP environment mode in some place like CompoundStmt so it gets set correctly when traversing the AST emitting IR? I'm not sure how TreeTransform would fit in here, though. The AST is supposed to be immutable once created, right? Can we set bits in a node after the node is created and before construction of the AST is completed?

Another wrinkle is that LLVM needs all FP in a function to be either constrained or non-constrained. This is to prevent code motion between the two modes. So clang would need to set the constrained mode someplace at function scope. I don't know the best place for that. And I'm not 100% sure that this requirement is set in stone yet. But where in the AST would be the proper place for that?

How does that change things?

wuzish added a subscriber: wuzish.Jul 3 2019, 7:35 PM
martong resigned from this revision.Jul 4 2019, 2:27 AM