| Message ID | 44C327AE-2328-4322-B792-61F22DFB1588@comcast.net |
|---|---|
| State | New |
| Headers | show |
On Mon, 9 Sep 2013, Mike Stump wrote: > Presently gcc just dies with a crash for an unhanded operation, the > below handles it better. > > I'm torn between sorry and error, error might be better. Thoughts? error means there is something wrong with the user's source code, and should generally be associated with the location of an erroneous source code construct. I don't see how it can be appropriate here; my impression is that this code should never fail for any compiler input, valid or invalid, and so an ICE seems better than sorry (which is for some well-defined source code feature lacking GCC support, or exceeding implementation limits in GCC).
On Sep 9, 2013, at 3:48 PM, "Joseph S. Myers" <joseph@codesourcery.com> wrote: > On Mon, 9 Sep 2013, Mike Stump wrote: > >> Presently gcc just dies with a crash for an unhanded operation, the >> below handles it better. >> >> I'm torn between sorry and error, error might be better. Thoughts? > > error means there is something wrong with the user's source code, Right. If one has mode X, and there are no instructions for mode X, the thing that is wrong with their source code is wanting to perform an operation in mode X. The language standard doesn't mandate that the operation for mode X works, so, we are free to just give an error. > should generally be associated with the location of an erroneous source > code construct. Indeed, the ^ points to exactly what is wrong in their source, which is (relatively new and) nice. > I don't see how it can be appropriate here; my impression > is that this code should never fail for any compiler input, Your impression is wrong. It does indeed fail.
On Sep 9, 2013, at 4:49 PM, Mike Stump <mikestump@comcast.net> wrote: >> I don't see how it can be appropriate here; my impression >> is that this code should never fail for any compiler input, Oh, I might have missed what you meant by this… If so, sorry… The nicest of options would be to generate a libcall for the missing operation… if we do that, then there would be no failure to compile and we can expect the runtime to put the operation in.
On Mon, 9 Sep 2013, Mike Stump wrote: > On Sep 9, 2013, at 3:48 PM, "Joseph S. Myers" <joseph@codesourcery.com> wrote: > > On Mon, 9 Sep 2013, Mike Stump wrote: > > > >> Presently gcc just dies with a crash for an unhanded operation, the > >> below handles it better. > >> > >> I'm torn between sorry and error, error might be better. Thoughts? > > > > error means there is something wrong with the user's source code, > > Right. If one has mode X, and there are no instructions for mode X, the > thing that is wrong with their source code is wanting to perform an > operation in mode X. The language standard doesn't mandate that the > operation for mode X works, so, we are free to just give an error. Well, your patch was missing the testcase, or explanation for why a testcase can't be added to the testsuite, so I don't know what sort of source code you have in mind here. But "mode" is only a source-code concept to the extent that the user uses "mode" attributes. If the user does a division of "int" values, say, that's fully defined by ISO C, and so if there's back-end support for it missing, that's a back-end bug and an ICE is appropriate; it doesn't reflect anything wrong with the user's source code. If however the user used a "mode" attribute for a mode for which the division operation is unsupported, the front end should give an error for that operation, just as the front end gives an error for dividing two struct values (unless of course that's supported by the language, in which case it gets lowered to appropriate GIMPLE); it doesn't generate GIMPLE with a struct division because that's invalid GIMPLE, and it shouldn't generate GIMPLE with a division on types for which the operation isn't supported. Note that for 32-bit x86, simply doing int x __attribute__((mode(TI))); gives "error: unable to emulate 'TI'"; you don't even need to try any operations on that mode. For target-specific types with more fine-grained restrictions on permitted operations, there are several target hooks such as invalid_unary_op, invalid_binary_op and invalid_parameter_type. That's how the errors should be given, so that the invalid GIMPLE is never generated. > > should generally be associated with the location of an erroneous source > > code construct. > > Indeed, the ^ points to exactly what is wrong in their source, which is > (relatively new and) nice. My observation has been that errors given by RTL expanders are at bad locations because input_location at that point tends to be set to the end of the function (at best). In particular, I've seen that for errors from target-specific built-in function expanders. But maybe this has improved since I saw such bad locations.
On Sep 10, 2013, at 3:43 PM, Joseph S. Myers <joseph@codesourcery.com> wrote: > For target-specific types with more fine-grained > restrictions on permitted operations, there are several target hooks such > as invalid_unary_op, invalid_binary_op and invalid_parameter_type. That's > how the errors should be given, so that the invalid GIMPLE is never > generated. Ah… yes, that does the trick, however, the disconnect between rtl and gimple is annoying. gimply (or rtl) is free to decompose operations, for example, xor can be decomposed into n independent xors of the parts of a larger piece of data, and if it does that, then the port should not give an error, and if gimple is does not do this, then the port should, but, the port can't know to do this or not, and to retain the flexibility to allow gimple lowering to improve over time. As a concrete example, xor, or, gt, ge, lt, le lower on plain integer modes; but eq and ne don't. Odd that. I'd claim it is a mere implementation detail of lowering and requiring port work for an internal implementation detail is odd. But, with the interfaces you mentioned I can solve the problem… I'll plan on doing it that way, not ideal, but reasonable. Thanks for the help.
diff --git a/gcc/expmed.c b/gcc/expmed.c index a83d549..127085f 100644 --- a/gcc/expmed.c +++ b/gcc/expmed.c @@ -4954,6 +4954,12 @@ expand_divmod (int rem_flag, enum tree_code code, enum machine_mode mode, } } + if (rem_flag ? (remainder == 0) : (quotient == 0)) + { + sorry ("operation not supported"); + return CONST0_RTX (mode); + } + return gen_lowpart (mode, rem_flag ? remainder : quotient); }