[PATCH] D83036: [X86][FPEnv] Lowering of {get,set,reset}_fpmode

Fri Mar 5 08:57:14 PST 2021

craig.topper added inline comments.

================
Comment at: llvm/test/CodeGen/X86/fpenv32.ll:16
+entry:
+  %v = call i32 @llvm.get.fpmode.i32()
+  ret i32 %v
----------------
kpn wrote:
> sepavloff wrote:
> > craig.topper wrote:
> > > sepavloff wrote:
> > > > craig.topper wrote:
> > > > > I'm still not clear who the intended user of these intrinsics are? How would they know when to use i32 and when to use i64?
> > > > > who the intended user of these intrinsics are?
> > > > 
> > > > These intrinsics can be useful at least in these cases:
> > > > * native implementation of `fegetmode`/`fesetmode`,
> > > > * internally by compiler when it implements pragmas changing control modes. Such pragma acts only within a compound statement and control modes should be restored upon exit.
> > > > 
> > > > > How would they know when to use i32 and when to use i64?
> > > > 
> > > > This is a platform-dependent choice, just as the size of pointers.  We could encode them in `DataLayout` (see D71741).
> > > > > who the intended user of these intrinsics are?
> > > > 
> > > > These intrinsics can be useful at least in these cases:
> > > > * native implementation of `fegetmode`/`fesetmode`,
> > > > * internally by compiler when it implements pragmas changing control modes. Such pragma acts only within a compound statement and control modes should be restored upon exit.
> > > 
> > > For the pragma case, would that mean the frontend would have to pass different values to the intrinsic for each target to match the target dependent behavior? Is the pragma also target specific? Or is there some target independent representation a frontend should be giving to llvm.
> > > 
> > > The ABI code in clang is a place where we have a ton of target specific rules in the frontend. It has come up many times on the mailing lists that this is bad design as every frontend needs to reimplement it. So I want to make sure we're being careful about the interface here and not creating work for frontends.
> > > 
> > > > 
> > > > > How would they know when to use i32 and when to use i64?
> > > > 
> > > > This is a platform-dependent choice, just as the size of pointers.  We could encode them in `DataLayout` (see D71741).
> > > 
> > > Based on the tests here it appears the size to use is different on 32-bit mode depending on whether sse is enabled. DataLayout can't be dependent on subtarget features. It's derived from the target triple.
> > > > > who the intended user of these intrinsics are?
> > > > 
> > > > These intrinsics can be useful at least in these cases:
> > > > * native implementation of `fegetmode`/`fesetmode`,
> > > > * internally by compiler when it implements pragmas changing control modes. Such pragma acts only within a compound statement and control modes should be restored upon exit.
> > > 
> > > For the pragma case, would that mean the frontend would have to pass different values to the intrinsic for each target to match the target dependent behavior?
> > 
> > Yes. The frontend now does the same things for `long` and some other types, it takes IR representation from DataLayout , and it is different for different architectures.
> > 
> > > Is the pragma also target specific?
> > 
> > `pragma STDC FENV_ROUND` is not target specific. Target-specific pragmas are of course possible, for example it could be a pragma that sets rounding mode for a particular type, if the target supports different modes for different types.
> > 
> > > Or is there some target independent representation a frontend should be giving to llvm.
> > 
> > For llvm properly generated variables are enough. It is frontend that need information, which can be supplied by DataLayout or TatgetInfo.
> > 
> > > 
> > > The ABI code in clang is a place where we have a ton of target specific rules in the frontend. It has come up many times on the mailing lists that this is bad design as every frontend needs to reimplement it. So I want to make sure we're being careful about the interface here and not creating work for frontends.
> > > 
> > 
> > The ABI is not touched here, these are intrinsics, they are not called as regular functions.
> > 
> > > > 
> > > > > How would they know when to use i32 and when to use i64?
> > > > 
> > > > This is a platform-dependent choice, just as the size of pointers.  We could encode them in `DataLayout` (see D71741).
> > > 
> > > Based on the tests here it appears the size to use is different on 32-bit mode depending on whether sse is enabled. DataLayout can't be dependent on subtarget features. It's derived from the target triple.
> > 
> > We can use the widest type for the mode set.
> > 
> > It is possible to use i64 for all targets, it would be enough for all targets supported by glibc. There are however functions `fesetenv` and `fegetenv` and in that case we cannot do similar thing as the size of FP state on X86 is 256 bits. 
> > 
> What pragmas are there that change control modes or the floating point environment? I don't think we have any, or at least none are required by C standards document. 
> > 
> > The ABI code in clang is a place where we have a ton of target specific rules in the frontend. It has come up many times on the mailing lists that this is bad design as every frontend needs to reimplement it. So I want to make sure we're being careful about the interface here and not creating work for frontends.
> > 
> 
> The ABI is not touched here, these are intrinsics, they are not called as regular functions.
> 

I only mentioned ABI because it is an example of a place where the interface between llvm and frontends is considered the wrong design. I wasn't trying to say that this affects ABI.

It sounds like for this intrinsic to be usable by a frontend, that any frontend that wants to use it needs to know exactly which bits represent rounding mode, what encoding they use, the fact that x86 has rounding mode bits in two places, etc.  That's a lot of information to try to pack into datalayout so likely it will have to just be hardcoded into custom C++ code in the frontend. Then when another frontend wants to use it they will have to write the same code into their frontend. And they'll need to update it every time they want to add support in their frontend for a new target that llvm already supports. This doesn't seem like a clean division between frontend and backend responsibilities.

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D83036