Page MenuHomePhabricator

cameron.mcinally (Cameron McInally)
User

Projects

User does not belong to any projects.

User Details

User Since
Jan 6 2015, 6:21 AM (211 w, 1 d)

Recent Activity

Mon, Jan 14

cameron.mcinally added a comment to D54649: [FPEnv] Rough out constrained FCmp intrinsics.

Bah, the following patch poked holes in this Diff:

Mon, Jan 14, 7:08 AM

Wed, Jan 9

cameron.mcinally added inline comments to D54649: [FPEnv] Rough out constrained FCmp intrinsics.
Wed, Jan 9, 9:09 AM

Tue, Jan 8

cameron.mcinally added inline comments to D54649: [FPEnv] Rough out constrained FCmp intrinsics.
Tue, Jan 8, 12:15 PM

Sat, Dec 29

cameron.mcinally added inline comments to D54649: [FPEnv] Rough out constrained FCmp intrinsics.
Sat, Dec 29, 11:43 AM

Dec 17 2018

cameron.mcinally added inline comments to D54649: [FPEnv] Rough out constrained FCmp intrinsics.
Dec 17 2018, 5:55 PM
cameron.mcinally added a comment to D55506: [RFC v2] Allow target to handle STRICT floating-point nodes.

Well, mayRaise Exception is purely a MI level flag. I struggle to see where optimizations on the MI level would ever care about rounding modes in the sense you describe: note that currently, MI optimizations don't even know which operation an MI instruction performs -- if you don't even know whether you're dealing with addition or subtraction, why would you care which rounding mode the operation is performed in? MI transformations instead care about what I'd call "structural" properties of the operation: what are the operands, what is input vs. output, which memory may be accessed, which special registers may involved, which other side effects may the operation have. This is the type of knowledge you need for the types of transformations that are done on the MI level: mostly about moving instructions around, arriving at an optimal schedule, de-duplicating identical operations performed multiple times etc. (Even things like simply changing a register operand to a memory operand for the same operation cannot be done solely by common MI optimizations but require per-target support.)

Dec 17 2018, 5:45 PM

Dec 14 2018

cameron.mcinally added a comment to D55506: [RFC v2] Allow target to handle STRICT floating-point nodes.

Well, this is because I'm really only using this feature to handle exceptions (b.t.w. just like the MemOperands in the alternate attempt).

Rounding mode is handled completely in the back-end: we simply make all floating-point instructions (strict or not doesn't even matter here) use the FPC control register, and mark all instructions that change the rounding mode as changing FPC. The one missing piece is that we need to mark all function calls to functions marked as within FENV_ACCESS ON regions as also clobbering FPC -- that can be done easily in the target ABI code as soon as the front-end marks such function calls (which we need anyway).

Dec 14 2018, 1:12 PM
cameron.mcinally added a comment to D55506: [RFC v2] Allow target to handle STRICT floating-point nodes.

@uweigand, it looks like you submitted some unintended changes with the last Diff update. Just FYI...

Dec 14 2018, 12:33 PM
cameron.mcinally added a comment to D54649: [FPEnv] Rough out constrained FCmp intrinsics.

A quiet ping on this, since this is my last working day of the year...

Dec 14 2018, 11:29 AM
cameron.mcinally added a comment to D55506: [RFC v2] Allow target to handle STRICT floating-point nodes.

I like this implementation a lot. Some targets control the FPEnv through a register, not through memory. Modeling instructions with register side-effects through MachineMemOperand wasn't intuitive.

Dec 14 2018, 8:25 AM

Dec 5 2018

cameron.mcinally added a comment to D54649: [FPEnv] Rough out constrained FCmp intrinsics.

Looking closer, I may have a better solution for the SETCC case. It won't solve the general problem of nodes needing to be Custom lowered though. Will report back soon...

Dec 5 2018, 9:41 AM
cameron.mcinally updated the diff for D54649: [FPEnv] Rough out constrained FCmp intrinsics.

Revert back to mutating a temp before Custom lowering.

Dec 5 2018, 9:40 AM

Dec 4 2018

cameron.mcinally added a comment to D54649: [FPEnv] Rough out constrained FCmp intrinsics.

The one problem with your "new" code is that it now forces back-ends to implement something, or else code involving constrained intrinsics will trigger internal compiler errors. It might be preferable to avoid those ...

Dec 4 2018, 9:17 AM
cameron.mcinally added a comment to D54649: [FPEnv] Rough out constrained FCmp intrinsics.

Old patch:

Dec 4 2018, 8:17 AM
cameron.mcinally added inline comments to D54649: [FPEnv] Rough out constrained FCmp intrinsics.
Dec 4 2018, 8:14 AM

Dec 3 2018

cameron.mcinally added inline comments to D53157: Teach the IRBuilder about constrained fadd and friends.
Dec 3 2018, 12:02 PM
cameron.mcinally added a comment to D53157: Teach the IRBuilder about constrained fadd and friends.

In my reading of the standard text, there is no special provision for library code. This means that in general, calling any library function while running with nondefault trap settings is undefined behavior, unless the library was itself compiled with FENV_ACCESS ON. There does not even appear to be any requirement for the C standard library functions to have been compiled with FENV_ACCESS ON, as far as I can see ...

Dec 3 2018, 11:24 AM
cameron.mcinally updated the diff for D54649: [FPEnv] Rough out constrained FCmp intrinsics.

Remove setting of STRICT_FSETCC Custom lowering, as @uweigand suggested.

Dec 3 2018, 10:58 AM
cameron.mcinally added inline comments to D54649: [FPEnv] Rough out constrained FCmp intrinsics.
Dec 3 2018, 10:56 AM
cameron.mcinally added inline comments to D54649: [FPEnv] Rough out constrained FCmp intrinsics.
Dec 3 2018, 9:37 AM
cameron.mcinally added inline comments to D54649: [FPEnv] Rough out constrained FCmp intrinsics.
Dec 3 2018, 8:09 AM

Nov 28 2018

cameron.mcinally added inline comments to D43515: More math intrinsics for conservative math handling.
Nov 28 2018, 10:59 AM
cameron.mcinally abandoned D50913: [FPEnv] Don't need copysign/fabs/fneg constrained intrinsics.

This issue was handled with the addition of a dedicated FNeg IR Instruction in D53877.

Nov 28 2018, 9:46 AM

Nov 27 2018

cameron.mcinally updated the diff for D54649: [FPEnv] Rough out constrained FCmp intrinsics.

@uweigand, what about something like this patch? The STRICT_FSETCC isn't handled all the way through the target, but it's stubbed out for now.

Nov 27 2018, 1:02 PM
cameron.mcinally updated the diff for D54649: [FPEnv] Rough out constrained FCmp intrinsics.

Update TableGen changes, as suggested by @nhaehnle.

Nov 27 2018, 10:10 AM
cameron.mcinally added a comment to D53157: Teach the IRBuilder about constrained fadd and friends.

Digressing a bit, but has anyone given thought to how this implementation will play out with libraries? When running with traps enabled, libraries must be compiled for trap-safety. E.g. optimizations on a lib's code could introduce a NaN or cause a trap that does not exist in the code itself.

Nov 27 2018, 7:56 AM

Nov 20 2018

cameron.mcinally added a comment to D53157: Teach the IRBuilder about constrained fadd and friends.

I think at this point we're all on the same page in this regard. I just wanted to make sure we also understand why we're on that page. I still believe it was the correct choice.

Nov 20 2018, 5:58 PM
cameron.mcinally added a comment to D53157: Teach the IRBuilder about constrained fadd and friends.

I'd like to hear more about this. The fesetexcept(...) function and friends change the FPEnv state, which can change the semantics for the instructions that follow. They definitely have to act as a barrier.

There's no sense in which we can have a code-motion barrier within a function that acts on the regular FP instructions. We can have a barrier for the constrained intrinsics. This is why we need, in this design, for any function that uses FENV_ACCESS=ON for any part of it, to always use the constrained instrinsics.

Nov 20 2018, 5:04 PM
cameron.mcinally added a comment to D53157: Teach the IRBuilder about constrained fadd and friends.

OK, let me try to expand on my point 3 above, which appears to have confused everybody :-)

Nov 20 2018, 4:58 PM
cameron.mcinally added a comment to D53157: Teach the IRBuilder about constrained fadd and friends.
In D53157#1304275, @kpn wrote:

If we all agree upon that, then we simply have to treat the functions that modify the FPEnv, e.g. fesetexcept(...), as barriers. That way it does not matter if a FENV_ACCESS=OFF function is translated with constrained intrinsics or not, since nothing can be scheduled around these barriers.

I thought we couldn't do barriers. No barriers means no way to prevent code motion and mixing of constrained with non-constrained FP. That was the reason for having all FP in a function be constrained if any of it was.

Nov 20 2018, 4:33 PM

Nov 19 2018

cameron.mcinally added a comment to D53157: Teach the IRBuilder about constrained fadd and friends.

A couple of comments on the previous discussion:

  1. Instead of defining a new command line option, I'd prefer to use the existing options -frounding-math and -ftrapping-math to set the default behavior of math operations w.r.t. rounding modes and exception status. (For compatibility with GCC if nothing else.)
Nov 19 2018, 1:14 PM

Nov 17 2018

cameron.mcinally added a comment to D53157: Teach the IRBuilder about constrained fadd and friends.

Just because FENV_ACCESS can be toggled on that granularity doesn't mean we have to represent it that way. We've previously agreed (I think) that if FENV_ACCESS is enabled anywhere in a function we will want to use the constrained intrinsics for all FP operations in the function, not just the ones in the scope where it was specified.

Yes, this is also my understanding. We can't soundly mix the two in the same function because we can't prevent the code motion within the function.

Nov 17 2018, 2:16 PM

Nov 16 2018

cameron.mcinally added a comment to D53157: Teach the IRBuilder about constrained fadd and friends.

Fair enough. Just for conversation's sake, I was envisioning the FE support a little differently...

Nov 16 2018, 3:17 PM
cameron.mcinally abandoned D54121: [FPEnv] Add constrained FCMP intrinsic.

Abandon this Revision for D54649...

Nov 16 2018, 1:56 PM
cameron.mcinally added inline comments to D54649: [FPEnv] Rough out constrained FCmp intrinsics.
Nov 16 2018, 1:50 PM
cameron.mcinally created D54649: [FPEnv] Rough out constrained FCmp intrinsics.
Nov 16 2018, 1:36 PM

Nov 15 2018

cameron.mcinally added a comment to D54121: [FPEnv] Add constrained FCMP intrinsic.

Oh, right. I missed that. So there should probably be a new STRICT_SETCC node. Thanks.

Nov 15 2018, 2:28 PM
cameron.mcinally added a comment to D54121: [FPEnv] Add constrained FCMP intrinsic.

I'm working on the changes that we discussed, but they're pretty invasive. A prospective patch is coming soon, but I wanted us to start thinking about how we'll handle these intrinsics at the SelectionDAG level. There are no CMP ISD nodes (also, what does legalization look like??), so this will likely be a significant change.

Nov 15 2018, 2:09 PM
cameron.mcinally updated the diff for D54549: [FNeg] Add FNeg Instruction to LangRef document.

Good catch, @arsenm. Thanks.

Nov 15 2018, 7:02 AM

Nov 14 2018

cameron.mcinally created D54549: [FNeg] Add FNeg Instruction to LangRef document.
Nov 14 2018, 3:25 PM
cameron.mcinally added a comment to D53877: [IR] Strawman for dedicated FNeg IR instruction.

I just realized the LangRef changes are missing

Nov 14 2018, 9:57 AM

Nov 13 2018

cameron.mcinally added a comment to D53877: [IR] Strawman for dedicated FNeg IR instruction.

I just realized the LangRef changes are missing

Nov 13 2018, 10:59 AM
cameron.mcinally added a comment to D53877: [IR] Strawman for dedicated FNeg IR instruction.

@majnemer has approved the changes offline. Will commit this patch imminently...

Nov 13 2018, 10:04 AM

Nov 12 2018

cameron.mcinally added a comment to D53877: [IR] Strawman for dedicated FNeg IR instruction.

David also gave a LGTM for the call-and-invoke-with-ranges.ll change off-line. If he's okay with the function code number change in LLVMBitCodes.h, I'll go ahead and commit this patch.

Nov 12 2018, 9:23 AM
cameron.mcinally updated the diff for D53877: [IR] Strawman for dedicated FNeg IR instruction.

Rebase and create new function code number, as suggested by @majnemer.

Nov 12 2018, 9:19 AM

Nov 9 2018

cameron.mcinally added a comment to D54121: [FPEnv] Add constrained FCMP intrinsic.

Thinking aloud...

Nov 9 2018, 10:06 AM
cameron.mcinally added a comment to D54121: [FPEnv] Add constrained FCMP intrinsic.

Here's a first whack at a list of operations needed internally:

Nov 9 2018, 9:26 AM

Nov 8 2018

cameron.mcinally updated the diff for D53877: [IR] Strawman for dedicated FNeg IR instruction.

Update test/Transforms/MergeFunc/call-and-invoke-with-ranges.ll to account for changes in the Instruction.def enum values.

Nov 8 2018, 2:30 PM
cameron.mcinally added a comment to D53157: Teach the IRBuilder about constrained fadd and friends.
In D53157#1291964, @kpn wrote:

I don't expect anyone would need to switch between constrained and regular floating point at an instruction level of granularity.

Nov 8 2018, 11:41 AM
cameron.mcinally added a comment to D54121: [FPEnv] Add constrained FCMP intrinsic.

The X86 builtin story is weird. There should be 9 builtins. I'm not sure how you found 12.

Nov 8 2018, 9:32 AM
cameron.mcinally added a comment to D53877: [IR] Strawman for dedicated FNeg IR instruction.

Wow, that's great. Thanks, Matt!

Nov 8 2018, 8:17 AM
cameron.mcinally added a comment to D54121: [FPEnv] Add constrained FCMP intrinsic.

More insight...

Nov 8 2018, 8:01 AM
cameron.mcinally added a comment to D54121: [FPEnv] Add constrained FCMP intrinsic.
compareSignalingGreaterEqual(a,b) is equivalent to compareSignalingLessUnordered(b, a)

Is it? If a or b is NaN the first one will return false but the second will return true.

Nov 8 2018, 7:21 AM

Nov 7 2018

cameron.mcinally added a comment to D54121: [FPEnv] Add constrained FCMP intrinsic.

That's cool with me. Definitely a ton of work, but it's clean and complete.

Nov 7 2018, 3:54 PM
cameron.mcinally updated the diff for D53877: [IR] Strawman for dedicated FNeg IR instruction.

Add bitcode tests for fastmath flags, as @arsenm suggested. Thanks for bearing with me, Matt.

Nov 7 2018, 3:24 PM
cameron.mcinally updated the diff for D53877: [IR] Strawman for dedicated FNeg IR instruction.

Add two test/Bitcode tests and address one llvm-bcanalyzer issue, as suggested by @arsenm.

Nov 7 2018, 2:43 PM
cameron.mcinally added a comment to D53877: [IR] Strawman for dedicated FNeg IR instruction.

Ah, I think I see some problems with llvm-bcanalyzer. Will address...

Nov 7 2018, 2:14 PM
cameron.mcinally added a comment to D53877: [IR] Strawman for dedicated FNeg IR instruction.

Bitcode compatibility tests are missing?

Nov 7 2018, 2:06 PM
cameron.mcinally updated the diff for D53877: [IR] Strawman for dedicated FNeg IR instruction.

Add newlines to lib/AsmParser/LLParser.cpp, as suggested by @arsenm.

Nov 7 2018, 1:24 PM
cameron.mcinally added a comment to D53157: Teach the IRBuilder about constrained fadd and friends.

Seems fair. I don't know enough about the Clang FE to have an opinion on that.

Nov 7 2018, 8:33 AM
cameron.mcinally updated subscribers of D53157: Teach the IRBuilder about constrained fadd and friends.

Adding the usual suspects: @andrew.w.kaylor @craig.topper @uweigand @hfinkel

Nov 7 2018, 8:22 AM
Herald added a reviewer for D52839: Inform AST's UnaryOperator of FENV_ACCESS: shafik.
Nov 7 2018, 8:06 AM

Nov 6 2018

cameron.mcinally added a comment to D54121: [FPEnv] Add constrained FCMP intrinsic.

All good points.

Nov 6 2018, 7:18 AM

Nov 5 2018

cameron.mcinally added inline comments to D54121: [FPEnv] Add constrained FCMP intrinsic.
Nov 5 2018, 2:06 PM
cameron.mcinally added inline comments to D54121: [FPEnv] Add constrained FCMP intrinsic.
Nov 5 2018, 1:51 PM
cameron.mcinally created D54121: [FPEnv] Add constrained FCMP intrinsic.
Nov 5 2018, 1:40 PM
cameron.mcinally added inline comments to D43515: More math intrinsics for conservative math handling.
Nov 5 2018, 11:51 AM

Nov 2 2018

cameron.mcinally added a comment to D43142: Experimental pass to convert all floating point operations to the equivalent constrained intrinsics.

We still need to control code motion. For instance, if the code checks the status bits before calling the FENV_ACCESS OFF function we can't have FP operations that are inlined hoisted above the status bit check.

Nov 2 2018, 2:51 PM
cameron.mcinally added a comment to D43142: Experimental pass to convert all floating point operations to the equivalent constrained intrinsics.

Well, yes and no. In a region defined as FENV_ACCESS off the user has granted the compiler explicit permission to ignore FP status bits and exception behavior. Depending on how we optimize these regions the FP status bits may come out differently and exceptions may or may not be raised, but the user has promised not to unmask exceptions or look at status bits in that region. So if that part of the behavior of the code changes depending on whether or not we inline that's OK because that's what the user signed up for. In that sense the behavior hasn't changed any more than it does between -O0 and -O3.

Nov 2 2018, 12:28 PM
cameron.mcinally added a comment to D43142: Experimental pass to convert all floating point operations to the equivalent constrained intrinsics.

The fact that we have chosen to inline the function doesn't change the semantics.

Nov 2 2018, 11:49 AM
cameron.mcinally added a comment to D43142: Experimental pass to convert all floating point operations to the equivalent constrained intrinsics.
In D43142#1285692, @kpn wrote:

Hmmm. A clang option to turn on FENV_ACCESS for entire compilations would work around this issue. So the behavior you are describing here would work for me.

Nov 2 2018, 10:57 AM
cameron.mcinally added a comment to D43142: Experimental pass to convert all floating point operations to the equivalent constrained intrinsics.
In D43142#1285565, @kpn wrote:

If all optimizations including constant folding, or at least optimizations on floating point, are delayed until after inlining then there's no problem.

Nov 2 2018, 10:08 AM
cameron.mcinally updated the diff for D53411: [FPEnv] Add constrained CEIL/FLOOR/ROUND/TRUNC intrinsics.

Rebase and remove extra '^' characters from docs/LangRef.rst.

Nov 2 2018, 9:49 AM
cameron.mcinally updated the diff for D53877: [IR] Strawman for dedicated FNeg IR instruction.

Add a comment about not reordering the llvm-c/Core.h enum values, as @jyknight suggested.

Nov 2 2018, 8:16 AM

Nov 1 2018

cameron.mcinally added a comment to D43142: Experimental pass to convert all floating point operations to the equivalent constrained intrinsics.

I thought what we said was that if constrained FP intrinsics were used anywhere in a function then they had to be used everywhere in the function. So if you inline a function with FENV_ACCESS=ON into a function with FENV_ACCESS=OFF then the FP operations in the destination function need to be converted to constrained intrinsics, and if you inline a function with FENV_ACCESS=OFF into a function with FENV_ACCESS=ON then the FP operations in the function being inlined need to be converted. This conversion will have a detrimental effect on optimizations, so it would be nice to factor that into the inling cost, but at this point I'm mostly concerned about generating correct code.

Nov 1 2018, 8:11 PM
cameron.mcinally added inline comments to D53877: [IR] Strawman for dedicated FNeg IR instruction.
Nov 1 2018, 1:31 PM
cameron.mcinally updated the diff for D53877: [IR] Strawman for dedicated FNeg IR instruction.

Don't reorder the members in llvm-c/Core.h as suggested by @jyknight.

Nov 1 2018, 12:05 PM
cameron.mcinally added a comment to D43142: Experimental pass to convert all floating point operations to the equivalent constrained intrinsics.
In D43142#1284296, @kpn wrote:

Then don't we have to deal with this sometime? If #pragma FENV_ACCESS ON is set for the main() example above then is constant folding allowed?

Nov 1 2018, 12:01 PM
cameron.mcinally added inline comments to D53877: [IR] Strawman for dedicated FNeg IR instruction.
Nov 1 2018, 11:52 AM
cameron.mcinally added a comment to D43142: Experimental pass to convert all floating point operations to the equivalent constrained intrinsics.
In D43142#1284190, @kpn wrote:

And couldn't we turn off constant folding in the IRBuilder when necessary?

Nov 1 2018, 11:24 AM
cameron.mcinally added a comment to D43142: Experimental pass to convert all floating point operations to the equivalent constrained intrinsics.

Here's a quick example. You can see in the IR that the ConstantFolder picks up the FDiv as undefined:

Nov 1 2018, 10:44 AM
cameron.mcinally added a comment to D43142: Experimental pass to convert all floating point operations to the equivalent constrained intrinsics.

I don't think this will work. The ConstantFolder could fold away traps early in the IRBuilder. We also wouldn't catch the FNeg/FSub case either.

Nov 1 2018, 10:34 AM
cameron.mcinally updated the diff for D53877: [IR] Strawman for dedicated FNeg IR instruction.

Remove extra whitespace from test/Assembler/fast-math-flags.ll.

Nov 1 2018, 10:26 AM
cameron.mcinally updated the diff for D53981: Fix whitespace in test/Assembler/fast-math-flags.ll .

Add -strict-whitespace option.

Nov 1 2018, 9:50 AM
cameron.mcinally added inline comments to D53877: [IR] Strawman for dedicated FNeg IR instruction.
Nov 1 2018, 9:24 AM
cameron.mcinally created D53981: Fix whitespace in test/Assembler/fast-math-flags.ll .
Nov 1 2018, 9:24 AM
cameron.mcinally updated the diff for D53877: [IR] Strawman for dedicated FNeg IR instruction.

Reorder LLVMAddrSpaceCast as @arsenm suggested.

Nov 1 2018, 8:39 AM
cameron.mcinally added inline comments to D53877: [IR] Strawman for dedicated FNeg IR instruction.
Nov 1 2018, 8:14 AM
cameron.mcinally updated subscribers of D53877: [IR] Strawman for dedicated FNeg IR instruction.
Nov 1 2018, 7:23 AM

Oct 31 2018

cameron.mcinally updated the diff for D53877: [IR] Strawman for dedicated FNeg IR instruction.

Updating diff to add test/Assembler tests. This includes both fastmath flags and double-round-tripping tests.

Oct 31 2018, 2:04 PM
cameron.mcinally added inline comments to D53877: [IR] Strawman for dedicated FNeg IR instruction.
Oct 31 2018, 11:31 AM
cameron.mcinally created D53932: [NFCI][FPEnv] Split constrained intrinsic tests.
Oct 31 2018, 7:17 AM

Oct 30 2018

cameron.mcinally added inline comments to D53216: [FPEnv] Add constrained intrinsics for MAXNUM and MINNUM.
Oct 30 2018, 2:05 PM
cameron.mcinally added a comment to D53877: [IR] Strawman for dedicated FNeg IR instruction.

with the intention of adding more Unary operations in the future. E.g. integer Neg (and maybe Abs, Copysign, etc.).

While there are very clear reasons for FNeg to exist, this isn't so true for any of the others listed here..

Oct 30 2018, 12:24 PM
cameron.mcinally added inline comments to D53877: [IR] Strawman for dedicated FNeg IR instruction.
Oct 30 2018, 11:16 AM
cameron.mcinally updated subscribers of D53877: [IR] Strawman for dedicated FNeg IR instruction.
Oct 30 2018, 11:15 AM
cameron.mcinally added inline comments to D53877: [IR] Strawman for dedicated FNeg IR instruction.
Oct 30 2018, 11:12 AM
cameron.mcinally created D53877: [IR] Strawman for dedicated FNeg IR instruction.
Oct 30 2018, 11:01 AM

Oct 25 2018

cameron.mcinally added a comment to D53216: [FPEnv] Add constrained intrinsics for MAXNUM and MINNUM.

Ping.

Oct 25 2018, 11:25 AM
cameron.mcinally updated the diff for D53650: [FPEnv] Last BinaryOperator::isFNeg(...) to m_FNeg(...) changes.

Whoops, forgot to update. Yeah, your vector test looks good now.

Oct 25 2018, 8:51 AM