Thanks for the review. Committed in r218064. On Thu, Sep 18, 2014 at 11:22 AM, Eric Christopher <[email protected]> wrote:
> 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
