This adds constrained intrinsics for the signed and unsigned conversions of integers to floating-point.
Details
Diff Detail
Event Timeline
This probably needs tests that will lower to single instructions. E.g. llvm.experimental.constrained.uitofp.v4f32.v4i32 should lower to a cvtps2dq on SSE2.
Maybe testing an AVX512VL target would be interesting too. They have really good support for different CVT variants.
llvm/lib/CodeGen/SelectionDAG/LegalizeDAG.cpp | ||
---|---|---|
1016 | Should these be clustered with STRICT_LRINT/etc? | |
2415 | Nit-picky: does this preserve the rounding results and flags raised? If the target doesn't support the itofp instruction, I'm not sure if we can do better anyway. Just wondering if anyone had thought this through... | |
2971 | Is the early return necessary here? It stands out from the surrounding code... |
For a first cut I was hoping to avoid dealing with Custom lowerings. That's why there's no v4* test cases. Is this something that can be handled when the X86 backend support gets to this point?
llvm/lib/CodeGen/SelectionDAG/LegalizeDAG.cpp | ||
---|---|---|
1016 | The regular U/SINT_TO_FP nodes are handled a couple of lines above. That makes these clustered close to the matching non-constrained versions, which seemed appropriate. Is there a better place? | |
2415 | Well, we don't have the metadata arguments at this point, so they can't be carried through to the extend/round instruction. And I don't know how a target that doesn't have an itofp instruction would handle a round instruction. Maybe? I don't know. | |
2971 | Yes, it's necessary. The normal path falls through to ReplaceNode() which can't handle, for example, replacing a node with two results with one that has only one. That's why we have to explicitly return the chain and then handle node replacement "by hand" here. You'll see this in other STRICT_* cases in this switch. |
Ah, right. I forgot about that. That's reasonable.
Besides the one inline comment, this all looks good. It's a big patch though, so I'll wait before accepting to give others time for review.
llvm/lib/CodeGen/SelectionDAG/LegalizeDAG.cpp | ||
---|---|---|
1016 | I think the STRICT_LRINT/etc cases should be moved up and combined with these new cases. All the non-strict variants are clustered together. It would make sense to cluster the strict variants directly under those. |
llvm/lib/CodeGen/SelectionDAG/LegalizeDAG.cpp | ||
---|---|---|
1016 | I suppose STRICT_LRINT/etc could be moved in a separate NFCI patch and then this patch just rebased. That's probably best. Sorry, just thinking aloud... |
Do we need a Verifier check to ensure that the vector element counts match between source and dest type? Not sure what we do for the other direction.
llvm/lib/CodeGen/SelectionDAG/LegalizeDAG.cpp | ||
---|---|---|
2343 | Ternary on the operand number? | |
2487 | Drop the else since we early returned? | |
2972 | Drop the else? | |
llvm/lib/CodeGen/SelectionDAG/SelectionDAG.cpp | ||
1126 | Is this longer than 80 columns? | |
llvm/lib/CodeGen/SelectionDAG/TargetLowering.cpp | ||
6122 | A ternary operator on the operand number would probably be shorter? | |
llvm/test/CodeGen/X86/fp-intrinsics.ll | ||
1946 | This check line is for IR, but this is an assembly test. CHECK isn't a valid check-prefix for this file. Which also means all of the CHECK-LABEL lines are broken in the existing tests aren't doing anything |
llvm/lib/CodeGen/SelectionDAG/LegalizeDAG.cpp | ||
---|---|---|
1016 | When one does something like this one notices that I'm calling TLI.getOperationAction() instead of TLI.getStrictFPOperationAction(). That seems like a bug in one of the two places. Hmmm. | |
llvm/lib/CodeGen/SelectionDAG/SelectionDAG.cpp | ||
1126 | Yes. I'll run the patch through clang-format before my next upload. | |
llvm/test/CodeGen/X86/fp-intrinsics.ll | ||
1946 | Ouch, yeah, that error is all over this file. How about I fix it for the new changes here and then fix the rest in another ticket? |
llvm/test/CodeGen/X86/fp-intrinsics.ll | ||
---|---|---|
1946 | What is the expected behavior here? We seem to doing some emulation of the operation instead of using the conversion instruction we have with SSE2. |
llvm/test/CodeGen/X86/fp-intrinsics.ll | ||
---|---|---|
1946 | That may be an artifact of of the getOperationAction()/getStrictFPOperationAction() issue that I found after the rearranging Cameron asked for. I'm looking into it now. |
It looks like I am going to have to do some Custom lowering work after all. Plus the changes to the IR verifier are missing.
llvm/test/CodeGen/X86/fp-intrinsics.ll | ||
---|---|---|
1946 | Yep, that's what caused it. If we use the getStrictFPOperationAction() route like the other STRICT nodes here then we end up with Custom lowerings. It's a shame there's no "Unassigned" "lowering". Currently there's no way to know if the target doesn't implement something or if it really does want to Expand like is currently returned. And LegalizeOp() doesn't bother trying to query the target like ExpandNode() does. That seems like a bug. |
llvm/test/CodeGen/X86/fp-intrinsics.ll | ||
---|---|---|
1946 | I didn't understand this. Isn't ExpandNode is only called from LegalizeOp? And doesn't LegalizeOp query the target to know to call ExpandNode? |
llvm/test/CodeGen/X86/fp-intrinsics.ll | ||
---|---|---|
1946 | Both true, yes. But for STRICT nodes LegalizeOp calls getStrictFPOperationAction(), which just calls getOperationAction() for the non-strict version of the node. So there's no way for LegalizeOp to know if the target wants to handle the STRICT node any differently from the non-strict. It never actually calls getOperationAction() for a STRICT node. |
llvm/test/CodeGen/X86/fp-intrinsics.ll | ||
---|---|---|
1946 | Maybe I'm confused here, but I thought I fixed this with my recent changes. These days, all code should always call getOperationAction on the strict node first, and only if that returns Expand, then it should call getStrictFPOperationAction to find out how the expansion is to be implemented. That means in LegalizeOp you should not call getStrictFPOperationAction. Really only ExpandNode should do so ... |
Address review comments. Add missing IR verifier pieces. Add the remaining test cases.
I've added the bare minimum X86 support to get the tests running.
llvm/lib/CodeGen/SelectionDAG/LegalizeDAG.cpp | ||
---|---|---|
1241 | Should we just be returning MERGE_VALUES from the custom handler for the strict fp nodes? That's how we handle loads and atomics. |
llvm/lib/CodeGen/SelectionDAG/LegalizeDAG.cpp | ||
---|---|---|
1241 | I'll take a look. Making this code simpler would be nice. |
llvm/docs/LangRef.rst | ||
---|---|---|
15703 | This sounds incomplete. I think some of the information from the arguments section ought to be here instead. | |
llvm/test/CodeGen/X86/fp-intrinsics.ll | ||
2004 | I feel like this should also be checking for the mov instruction, which implicitly zero-extends the 32-bit input. Maybe even a comment explaining what happened, because just looking at the test and seeing that uitofp gets lowered to cvtsi2sd looks wrong. | |
2022 | This sequence is quite opaque. I'm not sure it's the best way of handling this case, though I realize that's unrelated to your patch. What is related to your patch is the question of whether the subpd here might be introducing a spurious exception. I'd need to figure out what this is doing to answer that question. | |
2053 | Some more context would be useful here. I believe there is a test a jump and an addss that are also relevant in the output. |
I've switched the fp-intrinsic.ll test to use the update_llc_test_checks.py script and added more command lines in commit 774e829c29017d35e8af3b854f21c792caf30181. Ialso added more RUN lines to vector-constrained-fp-intrinsics.ll in 9e5116f756f05b68e8394e392027dca7bc574559 Can you rebase the tests here with those changes?
I like the switch to using update_llc_test_checks.py. Sure, I'll rebase, run that script and it'll be in the next round of this patch when I can.
llvm/test/CodeGen/X86/fp-intrinsics.ll | ||
---|---|---|
2053 | These issues are solved by switching to using the update_llc_test_checks.py script that Craig ran. I'll have it in my next round of this patch when I can get to it. |
Some comments on verifying the various algorithms for correctly respecting constrained FP semantics. I have not looked at the X86 back-end.
llvm/lib/CodeGen/SelectionDAG/LegalizeDAG.cpp | ||
---|---|---|
1016 | Yes, the code below for STRICT_LRINT etc seems buggy, it should definitely call TLI.getOperationAction instead. | |
2415 | I don't believe we need the metadata arguments here. Instead, we need to verify that this code sequence always rounds correctly according to the current rounding mode, no matter what it is, and also that this code sequence raises the same set of exceptions that a real IEEE compliant itofp operation would raise. So looking at the algorithm, it first constructs two double values "by hand", using only integer operations, which is always fine. Then those two are subtracted from one other. Due to the construction, neither can be NaN, and also the difference is always exactly representable. This means that the FSUB never raises any exception and does not depend on the current rounding mode, so this should also be fine. (So possibly, we don't even need a STRICT_FSUB here but can continue using a normal FSUB?) Finally, the result may be extended or rounded to another floating-point type. Extension should be fine since it never rounds and the input cannot be a NaN. Rounding should also be fine since it uses the correct (current) rounding mode, due to the construction of the input can never raise underflow or overflow exceptions, and raises the inexact exception exactly when it ought to be raised. | |
2428 | This is actually the more problematic case, because this may round the wrong way depending on the current rounding mode. For example, if the input is a large i64 value that does not exactly fit into the result type, and the rounding mode is "towards zero", then the result should be the FP value immediately smaller than the exact result. But since we're actually doing a signed conversion, the input will be interpreted as a negative value (of large absolute value), and rounded "towards zero", i.e. to the FP value immediately larger than the exact result of converting the negative value. After adding back the bias, we'll then have the FP value immediately larger than the exact result of an (unsigned) conversion of the original value, which is incorrect. It actually looks like this this has nothing to do with constrained FP semantics, this code simply gives the wrong result even for regular FP operations ... (GCC avoid this by either converting to a larger intermediate FP type first, if possible, or else avoiding the rounding issue by ensuring only positive values are passed to the signed-to-FP intermediate conversion.) | |
llvm/lib/CodeGen/SelectionDAG/LegalizeVectorOps.cpp | ||
1246 | This algorithm however looks fine to me. The SINT_TO_FP operations always operate on positive values, and even more so, since they only use a half-word, the result is guaranteed to fit exactly into the target FP type, so there is never any rounding or exception. (These then don't really need to be strict operations.) The FMUL only increments the exponent, so again there is no rounding or exception. The FADD at the end rounds correctly, and raises the correct exceptions, so this looks all good. | |
llvm/lib/CodeGen/SelectionDAG/LegalizeVectorTypes.cpp | ||
2627 | This confuses me again. It seems this may generate a SINT_TO_FP -> HalfVT -> FP_ROUND -> OutVT chain, which introduces a potential double rounding that can lead to incorrect results even disregarding any constrained FP semantics ... | |
llvm/lib/CodeGen/SelectionDAG/TargetLowering.cpp | ||
6165 | This also looks correct to me. The STRICT_SINT_TO_FP will round correctly in any rounding mode, if my understanding of the algorithm is correct, and it will also raise the inexact exception if appropriate. The STRICT_FADD is just a multiply by two, which does not depend on rounding and cannot raise any exceptions given the input (so it might as well be just a plain FADD). But what confuses me a bit again is why we have two algorithms for UINT_TO_FP expansion: a correct one here, and the incorrect one above in ExpandLegalINT_TO_FP? Under what circumstances will we ever end up using the incorrect one? | |
6217 | Same comment as above. |
llvm/lib/CodeGen/SelectionDAG/LegalizeDAG.cpp | ||
---|---|---|
2965–2966 | Extra indentation | |
llvm/lib/CodeGen/SelectionDAG/SelectionDAG.cpp | ||
1122 | I think the input chain should be an argument here if we're going to make this a reusable function. Op could theoretically be a node with 2 data results and a chain which would make this wrong. It should probably also return std::pair of the two results so the caller doesn't have to assume where the chain is for nodes that were created inside. |
llvm/lib/CodeGen/SelectionDAG/LegalizeVectorTypes.cpp | ||
---|---|---|
2627 | For the only case we have tests for i64->f16. I think any integer value large enough to cause rounding when converted to f32 would be too large to represent at all in f16. Since f16's max exponent of 15 is less than the length of f32's mantissa. |
llvm/lib/CodeGen/SelectionDAG/LegalizeVectorTypes.cpp | ||
---|---|---|
2627 | Ah yes, you're right. So this should be fine with non-strict semantics. And for strict semantics, we should also be fine. The int->f32 conversion can only raise an inexact exception, and this only in cases where the int->f16 conversion should raise an inexact. The f32->f16 conversion due to construction of the input also can only raise an inexact exception, and again only in cases where we should have one. Conversely, in every case where we should have an inexact exception, one (or both) of the intermediate steps will raise it. (I think there may be cases where we get two, but that should be fine even for strict semantics.) |
llvm/lib/CodeGen/SelectionDAG/LegalizeVectorTypes.cpp | ||
---|---|---|
2627 | Following up on myself: of course the f32->f16 conversion can also overflow, but again only in cases (and in fact in exactly in those cases) where the original int->f16 conversion should have overflowed. So again this should be fine. Sorry again for the confusion, I'm not really used to thinking about f16 :-) |
llvm/lib/CodeGen/SelectionDAG/LegalizeVectorTypes.cpp | ||
---|---|---|
2627 | We should probably fix the code only do this for f16. I think we could construct a vXi128->vXf32 test that would go through this and I think it would be wrong. |
llvm/lib/CodeGen/SelectionDAG/LegalizeVectorTypes.cpp | ||
---|---|---|
2627 | Nevermind, I think we don't use the algorithm if the input type will be scalarized. I expect vXi128 will be scalarized on all targets. |
Update for review comments. Use MERGE_VALUES node. Eliminate hacks to avoid x86 custom lowering. This means more x86 target support.
I have not addressed the algorithm issues pointed out by Ulrich. So far I'm trying to just get the existing algorithms converted for strict use. Would it make sense to hold off on algorithm changes until another patch?
llvm/lib/Target/X86/X86ISelLowering.cpp | ||
---|---|---|
996 | Line the arguments up with the l ine above. Same for the two below | |
18550–18551 | Please add a FIXME | |
18737 | getValue | |
18738 | getValue | |
18759 | You can DAG.getMergeValues which takes less arguments | |
18806 | Don't we need to propagate the chain result too? | |
18826 | Pleas add a FIXME here |
llvm/lib/Target/X86/X86ISelLowering.cpp | ||
---|---|---|
18806 | When given a strict FP node LowerOperation() will require that the resulting new node has the chain in the same place in the values. So, no, I don't think so unless we want to thread the std::pair all the way back up. |
llvm/lib/Target/X86/X86ISelLowering.cpp | ||
---|---|---|
18806 | Oh this works because the node is either a STRICT_FP_EXTEND or STRICT_FP_ROUND so the node in first and second is the same. But we should probably use a MERGE_VALUES here to not rely on that. |
llvm/lib/CodeGen/SelectionDAG/SelectionDAG.cpp | ||
---|---|---|
7374 | This only works if N2 is result 1 of N1. |
For me this would be fine as a first step, but we should at least add FIXME statements to those algorithms where we already know there are strict semantics problems.
llvm/include/llvm/CodeGen/TargetLowering.h | ||
---|---|---|
4125–4126 | Should document Chain here. Or maybe it would be better to use MERGE_VALUES to return both components if appropriate? | |
llvm/lib/CodeGen/SelectionDAG/LegalizeDAG.cpp | ||
175–176 | If we pass Node, then all other arguments are really redundant, aren't they? | |
176–178 | Hmm, wouldn't we also need to update this routine? Or can we say that promotion is not appropriate for handling strict semantics anyway? | |
2964 | Why do we have to do the Replace... thing here instead of just appending both to Results (like is done for other nodes with chain)? | |
llvm/lib/CodeGen/SelectionDAG/LegalizeVectorOps.cpp | ||
327–328 | This should now also use ValVT, I think. | |
llvm/lib/CodeGen/SelectionDAG/SelectionDAG.cpp | ||
9202 | Hmm. There's an UnrollVectorOp_StrictFP in LegalizeVectorTypes.cpp. If we change this routine to also handle some strict operations, maybe we should go all the way and just have merge the routines completely? | |
llvm/lib/IR/Verifier.cpp | ||
4810 | I believe the NumOperands check is now redunant with common tests handled above. |
llvm/lib/CodeGen/SelectionDAG/SelectionDAG.cpp | ||
---|---|---|
7374 | I don't understand. N2 is a constant that is either 0 or 1. What will happen if it is discarded here? This code was lifted straight out of getNode() somewhere around line 5194. Without it the X86 target dies trying to lower a rounding of f64 to f64. This happens because getStrictFPExtendOrRound() returns a round when input and output are the same size. This mirrors the non-strict getFPExtendOrRound(). |
llvm/lib/CodeGen/SelectionDAG/SelectionDAG.cpp | ||
---|---|---|
7374 | Oops you’re right about N2. But there is an issue with the chain. Which I think is what I was thinking N2 was when I wrote that. If the input chain didn’t come from N1 this is broken. I don’t see any precedent for returning a MERGE_VALUES from getNode. I think we need to fix the caller of getStrictFPExtendOrRound to only call when necessary. |
-Improve some of the X86 code.
-Add Promote support. Use it for i8/i16 on X86.
-Remove changes to UnrollVectorOp which seemed to be unexercised
-Some cleanup in ExpandLegalINT_TO_FP
-Drop the changes to getNode. We can't fold NOP conversions here and asserts were recently added in another patch.
Change the signature of SelectionDAGLegalize::ExpandLegalINT_TO_FP to allow it to update the Results vector directly.
I'm still working on this ticket daily! I'm trying to merge the two vector unrolling functions like Ulrich suggested. But I ran into problems that lead me to think we may have a serious issue lurking that we'll need to fix. That's what I've been working on: trying to understand the issue and see if it needs further investigation.
If you are in a hurry then you could have sent me an email and I would have uploaded the diffs I've got without further investigation.
I'm leaving attached the comments on my work that I've been adding but haven't submitted until now.
llvm/include/llvm/CodeGen/TargetLowering.h | ||
---|---|---|
4125–4126 | Documentation. Check. I'll have that in my next round. I don't remember why the prototype for expandFP_TO_UINT() got reformatted. But this code is already in the tree. Having expandUINT_TO_FP() be consistent with the existing tree seems like a good idea? | |
llvm/lib/CodeGen/SelectionDAG/LegalizeDAG.cpp | ||
175–176 | Seems that way. I'll give it a try. | |
176–178 | I don't have a test for it so I didn't change it. Can we guarantee the result would be rounded back down? Seems like promotion would be invalid without that guarantee. | |
2964 | I've had a lot of trouble with this, but at the moment I'm unable to reproduce any issue here. Let's simplify and see how it goes. | |
llvm/lib/CodeGen/SelectionDAG/LegalizeVectorOps.cpp | ||
327–328 | Looks like it. | |
llvm/lib/CodeGen/SelectionDAG/SelectionDAG.cpp | ||
7374 | Done. I've also added an assert to document that getStrictFPExtendOrRound() wants the lengths to be different. |
Sorry. I should have sent a mail. If you want to upload over my last patch, I can merge my X86 changes into it.
What’s the serious issue you’ve encountered?
The calls DAG.UnrollVectorOp() can't be used in place of DAGTypeLegalizer::ReplaceValueWith() because then the replaced node doesn't get properly deleted. This then results in an assertion failure later because the replaced node wasn't legal. And having SelectionDAG call into DAGTypeLegalizer doesn't seem like a winning idea, at least not without some plumbing. So today I'm going to punt on merging those functions together and just post what I've got. After breakfast sometime.
I've looked over the common code changes again, and they now look good to me. I'll leave it to Craig to review the X86 changes ...
llvm/lib/CodeGen/SelectionDAG/LegalizeDAG.cpp | ||
---|---|---|
176–178 | Right. In general promotion is not appropriate for strict semantics because you don't get the right exceptions (for overflow etc.). | |
2396–2414 | NewChain seems superfluous? |
llvm/lib/CodeGen/SelectionDAG/LegalizeDAG.cpp | ||
---|---|---|
2396–2414 | Right, fixed now. Thanks! |
llvm/lib/CodeGen/SelectionDAG/LegalizeDAG.cpp | ||
---|---|---|
176–178 | Promotion is definitely bad for fp->int, but is there really an issue for int->fp? We're just going to use a bigger int for the input. If the small int was going to overflow, it should still overflow when its extended. |
llvm/lib/CodeGen/SelectionDAG/LegalizeVectorOps.cpp | ||
---|---|---|
1160–1163 | Can a strict node get here? The call site in vectorLegalizer::Expand only checks for the non-strict node. | |
llvm/lib/CodeGen/SelectionDAG/SelectionDAG.cpp | ||
9202 | When are these changes needed? The STRICT_UINT_TO_FP handling in LegalizeVectorOps always scalarizes and goes through ExpandStrictFPOp. |
llvm/lib/CodeGen/SelectionDAG/LegalizeDAG.cpp | ||
---|---|---|
2396 | Can we merge the 2 strict fp ifs here? The only thing we do between them is declare a new variable. | |
2398 | Why is this not just DestVT != Sub.getValueType() | |
llvm/lib/Target/X86/X86ISelLowering.cpp | ||
18744 | We probably do need a strict version of FHADD, but until we have that we should just go to the shuffle + STRICT_FADD code below rather than silently dropping the chain. |
llvm/lib/Target/X86/X86ISelLowering.cpp | ||
---|---|---|
19026–19032 | This looks out of date. I recently changed this to return a std::pair of Result and Chain so it was obvious that the chain result was intended as an output. So we need a merge values here for strict fp now. This should be reflected in https://reviews.llvm.org/D71130 |
Huh. I forgot to submit my comments earlier.
Anyway, I've merged in many changes from D71130, I've made a few changes as requested in LegalizeDAG.cpp, and I added the missing call to ExpandUINT_TO_FLOAT(). Now I'm seeing X86::VMULPDYrr die when running the vector-constrained-fp-intrinsics.ll test on the v4i32 type. It seems that in InstrEmitter.cpp it dies at the assertion at line 838 because NumOperands is 3. That's as far as I've gotten today.
llvm/lib/CodeGen/SelectionDAG/LegalizeDAG.cpp | ||
---|---|---|
176–178 | Agreed. I'm working on LegalizeDAG.cpp right now. I notice X86 already has Promote for i8 and i16. | |
2396 | Done. | |
2398 | Eh, I figured it'd be more clear this way. I'll change it. | |
llvm/lib/CodeGen/SelectionDAG/LegalizeVectorOps.cpp | ||
1160–1163 | Ah, I must have lost the change to call here, possibly with D69887 going in. | |
llvm/lib/Target/X86/X86ISelLowering.cpp | ||
18744 | Change lifted from D71130. | |
19026–19032 | Yup, I just needed to rebase. |
Address review comments, with changes to LegalizeDAG.cpp and LegalizeVectorOps.cpp.
One comment unaddressed still.
llvm/lib/CodeGen/SelectionDAG/LegalizeVectorOps.cpp | ||
---|---|---|
1209 | That was it. Thanks! |
llvm/lib/CodeGen/SelectionDAG/SelectionDAG.cpp | ||
---|---|---|
9202 | VectorLegalizer::ExpandUINT_TO_FLOAT() uses UnrollVectorOp() if the required instructions are not available. I have a change where it gets called from the top of ExpandStrictFPOp() and instead returns an empty value if it would have called UnrollVectorOp(). I think that may eliminate the need to modify UnrollVectorOp() since it will then fall through into the ExpandUINT_TO_FLOAT() logic instead. If it works it'll generate better code, and eliminating complexity would be nice. |
Address review comments. I've eliminated the need to call SelectionDAG::UnrollVectorOp() and therefore eliminated the changes to that function.
llvm/lib/CodeGen/SelectionDAG/LegalizeVectorOps.cpp | ||
---|---|---|
1359–1364 | I addressed this by eliminating the need to call DAG.UnrollVectorOp(). That function is now out of the discussion. The tests show that this way here usually results in shorter sequences of instructions as well. |
General question for you and @uweigand that I realized today. Do we need to set the FPExcept bit in the flags for new nodes when we expand/promote operations?
llvm/lib/CodeGen/SelectionDAG/LegalizeVectorOps.cpp | ||
---|---|---|
1182 | Isn't SDValue(nullptr, 0) equivalent to SDValue() and more consistent with other code? | |
1361 | Can we just declare SDValue Res in the if condition so we don't need double parentheses? |
Ah yes, we need to do that.
This reminds me of a general concern: for all other flag bits, omitting the flag is conservatively safe, it just may impact performance. But for FPExcept, omitting the flag impacts correctness. Maybe we should go ahead and invert the sense of the flag (i.e. use a FPNoExcept flag instead of FPExcept) after all ...
This sounds incomplete. I think some of the information from the arguments section ought to be here instead.