LGTM. Thanks! -eric
On Thu, Sep 18, 2014 at 11:17 AM, Akira Hatanaka <[email protected]> wrote: > The attached patch is a follow-up to r217994. I defined a new function > validateOperandSize, which is used to check both input and output sizes. > > > On Tue, Sep 16, 2014 at 6:07 PM, Eric Christopher <[email protected]> > wrote: > >> Cool, thanks. >> >> -eric >> >> On Tue, Sep 16, 2014 at 6:02 PM, Akira Hatanaka <[email protected]> >> wrote: >> >>> OK, I'll check in a patch that fixes X86_32TargetInfo::validateInputSize >>> first then. >>> >>> On Tue, Sep 16, 2014 at 5:55 PM, Eric Christopher <[email protected]> >>> wrote: >>> >>>> >>>> >>>> On Tue, Sep 16, 2014 at 5:53 PM, Akira Hatanaka <[email protected]> >>>> wrote: >>>> >>>>> On Tue, Sep 16, 2014 at 5:12 PM, Eric Christopher <[email protected]> >>>>> wrote: >>>>> >>>>>> >>>>>> >>>>>> On Tue, Sep 16, 2014 at 4:00 PM, Akira Hatanaka <[email protected]> >>>>>> wrote: >>>>>> >>>>>>> On Tue, Sep 16, 2014 at 1:06 PM, Eric Christopher < >>>>>>> [email protected]> wrote: >>>>>>> >>>>>>>> You'll want to split out the new contraints for input size into a >>>>>>>> separate patch. (And just commit it). >>>>>>>> A small comment of why we're ignoring dependent types would be good. >>>>>>>> >>>>>>>> One question: Why not just add all of the contraints first rather >>>>>>>> than piecemeal as you get testcases? (Related to the comment above). >>>>>>>> >>>>>>>> >>>>>>> Just to make sure I'm not misunderstanding your question, are you >>>>>>> suggesting I use "=abcdSD" instead of "=a" in the test case and do the >>>>>>> check in one line? >>>>>>> >>>>>>> uint64_t val; >>>>>>> >>>>>>> __asm__ volatile("addl %1, %0" : "=abcdSD" (val) : "a" (msr)); // >>>>>>> expected-error {{invalid output size for constraint '=abcdSD'}} >>>>>>> >>>>>>> >>>>>>> Are you also suggesting that we should have clang print just the >>>>>>> constraints that are invalid in the error message? For example, if we >>>>>>> added >>>>>>> "A" and use "=abcdSDA" instead, clang would remove "A", since it can be >>>>>>> bound to a 64-bit variable, and print "=abcdSD" or "abcdSD" instead? >>>>>>> >>>>>>> >>>>>>> >>>>>> No, I'm curious why you're adding S and D now, but not any other >>>>>> constraint that has a size associated with the register. >>>>>> >>>>>> >>>>> OK, I see. I just felt that S and D should be added too, since they >>>>> are single register constraints that have to be bound to variables smaller >>>>> than 64-bit, as constraints a-d are. >>>>> >>>>> I can probably add R, q, Q, to the switch-case statement too. Also, in >>>>> my next patch, I was going to add checks for constraints x and y. >>>>> >>>>> Should I add the all the constraints I mentioned above to >>>>> X86_32TargetInfo::validateInputSize or X86TargetInfo::validateInputSize >>>>> first and then add the checks for output constraints? >>>>> >>>> >>>> Seems like a reasonable way to go yes? >>>> >>>> -eric >>>> >>>> >>>>> >>>>> -eric >>>>>> >>>>>> >>>>>>> >>>>>>> Thanks! >>>>>>>> >>>>>>>> -eric >>>>>>>> >>>>>>>> On Fri Aug 29 2014 at 4:46:37 PM Akira Hatanaka <[email protected]> >>>>>>>> wrote: >>>>>>>> >>>>>>>>> Does the latest patch look fine? I am working on another patch >>>>>>>>> which fixes a similar bug and I need to commit this patch first. >>>>>>>>> >>>>>>>>> >>>>>>>>> On Thu, Aug 28, 2014 at 10:08 AM, Akira Hatanaka < >>>>>>>>> [email protected]> wrote: >>>>>>>>> >>>>>>>>>> Latest version of the patch is attached which fixes a couple of >>>>>>>>>> oversights. I had to add a line which checks whether Ty is a >>>>>>>>>> dependent type >>>>>>>>>> before getTypeSize is called. Also, in the test case, "=" was missing >>>>>>>>>> before constraint "a", so fixed that too. >>>>>>>>>> >>>>>>>>>> On Wed, Aug 27, 2014 at 3:22 PM, Reid Kleckner <[email protected]> >>>>>>>>>> wrote: >>>>>>>>>> >>>>>>>>>>> New patch looks good to me. >>>>>>>>>>> >>>>>>>>>>> It sounds like we have two cases of size mismatch: >>>>>>>>>>> - The output operand lvalue is smaller than the constraint, >>>>>>>>>>> meaning the store will write out of bounds. Your patch adds this. >>>>>>>>>>> - The output operand lvalue is bigger than the constraint, >>>>>>>>>>> meaning the whole value won't be initialized. We currently warn >>>>>>>>>>> here via >>>>>>>>>>> validateConstraintModifier. >>>>>>>>>>> >>>>>>>>>>> This code probably deserves some cleanup, but your patch is >>>>>>>>>>> consistent with what we do for input operands, so let's go with >>>>>>>>>>> that. >>>>>>>>>>> >>>>>>>>>>> >>>>>>>>>> The reason llvm is crashing in the backend is that it's trying to >>>>>>>>>> use a 64-bit register in 32-bit mode. It's not because a store is >>>>>>>>>> writing >>>>>>>>>> out of bounds or there is a value left uninitialized. In the test >>>>>>>>>> case, if >>>>>>>>>> we declare the variable bound to constraint "=a" to be a unit32_t or >>>>>>>>>> an >>>>>>>>>> integer type that is smaller than 32-bit, clang compiles the program >>>>>>>>>> fine. >>>>>>>>>> >>>>>>>>>> >>>>>>>>>>> >>>>>>>>>>> On Wed, Aug 27, 2014 at 12:35 PM, Akira Hatanaka < >>>>>>>>>>> [email protected]> wrote: >>>>>>>>>>> >>>>>>>>>>>> The commit log in r166737 doesn't say much about why this is a >>>>>>>>>>>> warning instead of an error, but I know there are cases where >>>>>>>>>>>> warnings are >>>>>>>>>>>> needed. For example, clang has to issue warnings instead of errors >>>>>>>>>>>> for the >>>>>>>>>>>> inline-asm statements in the test case committed in r216260. If >>>>>>>>>>>> it's not >>>>>>>>>>>> desirable to change validateConstraintModifier, we can add a >>>>>>>>>>>> function which >>>>>>>>>>>> checks the output size that is similar to validateInputSize in >>>>>>>>>>>> r167717 (see >>>>>>>>>>>> attached patch), which was suggested in the post-commit review. >>>>>>>>>>>> >>>>>>>>>>>> >>>>>>>>>>>> http://lists.cs.uiuc.edu/pipermail/cfe-commits/Week-of-Mon-20121112/067945.html >>>>>>>>>>>> >>>>>>>>>>>> I am not sure whether we can use fixit in this case. Fixit >>>>>>>>>>>> hints should be used only if we know the user's intent and it's >>>>>>>>>>>> very clear >>>>>>>>>>>> that applying the fixit hint is the right thing to do. Changing >>>>>>>>>>>> the type of >>>>>>>>>>>> variable "r" to a 32-bit int will avoid crashing, but it doesn't >>>>>>>>>>>> look like >>>>>>>>>>>> that's what the user wants. >>>>>>>>>>>> >>>>>>>>>>>> On Mon, Aug 25, 2014 at 6:20 PM, Reid Kleckner <[email protected]> >>>>>>>>>>>> wrote: >>>>>>>>>>>> >>>>>>>>>>>>> Can you investigate why we are warning in the first place? I >>>>>>>>>>>>> think we should either only warn or only error. Currently we have >>>>>>>>>>>>> a warning >>>>>>>>>>>>> with a fixit but we don't recover as though we had applied the >>>>>>>>>>>>> fixit. If we >>>>>>>>>>>>> did that, we would not crash. >>>>>>>>>>>>> >>>>>>>>>>>>> In addition to the Clang-side changes, LLVM should probably be >>>>>>>>>>>>> returning an error or reporting a fatal error instead of hitting >>>>>>>>>>>>> unreachable. >>>>>>>>>>>>> >>>>>>>>>>>>> >>>>>>>>>>>>> On Mon, Aug 25, 2014 at 2:10 PM, Akira Hatanaka < >>>>>>>>>>>>> [email protected]> wrote: >>>>>>>>>>>>> >>>>>>>>>>>>>> Rebased patches attached. >>>>>>>>>>>>>> >>>>>>>>>>>>>> I also made changes to the clang patch so that clang can >>>>>>>>>>>>>> error-out after a size mismatch is found as soon as >>>>>>>>>>>>>> possible.TargetInfo::validateConstraintModifier has an extra >>>>>>>>>>>>>> parameter >>>>>>>>>>>>>> IsError, which is set when it decides there is no point in >>>>>>>>>>>>>> continuing >>>>>>>>>>>>>> compilation and it should stop compilation immediately. The >>>>>>>>>>>>>> error message >>>>>>>>>>>>>> clang prints looks better than lllvm's message, but if it isn't >>>>>>>>>>>>>> right to >>>>>>>>>>>>>> change the warning to an error, then I guess we have to detect >>>>>>>>>>>>>> the error >>>>>>>>>>>>>> later just before isel, as is done in the llvm patch. >>>>>>>>>>>>>> >>>>>>>>>>>>>> On Wed, Jul 23, 2014 at 3:56 PM, Akira Hatanaka < >>>>>>>>>>>>>> [email protected]> wrote: >>>>>>>>>>>>>> >>>>>>>>>>>>>>> llvm should error-out when a 64-bit variable is bound to a >>>>>>>>>>>>>>> single register in x86 32-bit mode, but ToT clang/llvm fails to >>>>>>>>>>>>>>> detect this >>>>>>>>>>>>>>> error and continues compilation until it crashes in >>>>>>>>>>>>>>> type-legalization: >>>>>>>>>>>>>>> >>>>>>>>>>>>>>> $ llc test/CodeGen/X86/inline-asm-regsize.ll -O3 >>>>>>>>>>>>>>> -mtriple=i386-apple-darwin -o - >>>>>>>>>>>>>>> >>>>>>>>>>>>>>> inline-asm-regsize.ll -O3 -mtriple=i386-apple-darwin -o - >>>>>>>>>>>>>>> >>>>>>>>>>>>>>> .section __TEXT,__text,regular,pure_instructions >>>>>>>>>>>>>>> >>>>>>>>>>>>>>> ExpandIntegerResult #0: 0x7fa2d1041728: i64 = Register %RCX >>>>>>>>>>>>>>> [ID=0] >>>>>>>>>>>>>>> >>>>>>>>>>>>>>> >>>>>>>>>>>>>>> Do not know how to expand the result of this operator! >>>>>>>>>>>>>>> >>>>>>>>>>>>>>> UNREACHABLE executed at >>>>>>>>>>>>>>> /Users/ahatanaka/projects/llvm/git/llvm3/lib/CodeGen/SelectionDAG/LegalizeIntegerTypes.cpp:1116! >>>>>>>>>>>>>>> >>>>>>>>>>>>>>> The attached patch fixes llvm to error-out and print this >>>>>>>>>>>>>>> error message: >>>>>>>>>>>>>>> >>>>>>>>>>>>>>> error: Cannot bind a variable larger than 32-bit to a single >>>>>>>>>>>>>>> register in 32-bit mode >>>>>>>>>>>>>>> >>>>>>>>>>>>>>> >>>>>>>>>>>>>>> My initial solution was to have clang detect this error in >>>>>>>>>>>>>>> TargetInfo::validateConstraintModifier. However, the code in >>>>>>>>>>>>>>> SemaStmtAsm.cpp has to be changed to error-out instead of >>>>>>>>>>>>>>> issuing a >>>>>>>>>>>>>>> warning, which I wasn't sure was the right thing to do. I am >>>>>>>>>>>>>>> attaching this >>>>>>>>>>>>>>> patch too in case someone has a suggestion or an opinion on it. >>>>>>>>>>>>>>> >>>>>>>>>>>>>>> <rdar://problem/17476970> >>>>>>>>>>>>>>> >>>>>>>>>>>>>>> >>>>>>>>>>>>>> >>>>>>>>>>>>>> _______________________________________________ >>>>>>>>>>>>>> cfe-commits mailing list >>>>>>>>>>>>>> [email protected] >>>>>>>>>>>>>> http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits >>>>>>>>>>>>>> >>>>>>>>>>>>>> >>>>>>>>>>>>> >>>>>>>>>>>> >>>>>>>>>>> >>>>>>>>>> >>>>>>>>>> >>>>>>>>>> >>>>>>>>> _______________________________________________ >>>>>>>>> cfe-commits mailing list >>>>>>>>> [email protected] >>>>>>>>> http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits >>>>>>>>> >>>>>>>> >>>>>>> >>>>>> >>>>> >>>> >>> >> >
_______________________________________________ cfe-commits mailing list [email protected] http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
