[llvm] r224901 - [X86][ISel] Fix a regression I introduced in r224884

[Hal Finkel]

----- Original Message -----
> From: "H.J. Lu" <hjl.tools at gmail.com>
> To: "Keno Fischer" <kfischer at college.harvard.edu>
> Cc: "Hal Finkel" <hfinkel at anl.gov>, "Commit Messages and Patches for LLVM" <llvm-commits at cs.uiuc.edu>
> Sent: Monday, December 29, 2014 7:22:52 AM
> Subject: Re: [llvm] r224901 - [X86][ISel] Fix a regression I introduced in r224884
> 
> On Mon, Dec 29, 2014 at 2:41 AM, Keno Fischer
> <kfischer at college.harvard.edu> wrote:
> > Ok, what's the general rule on whether to include `target triple`
> > and
> > `target datalayout` in the tests? Also, H.J. could you comment on
> > why you
> > recommend omitting it.
> >
> > On Sun, Dec 28, 2014 at 11:06 PM, Hal Finkel <hfinkel at anl.gov>
> > wrote:
> >>
> >> ----- Original Message -----
> >> > From: "Keno Fischer" <kfischer at college.harvard.edu>
> >> > To: llvm-commits at cs.uiuc.edu
> >> > Sent: Sunday, December 28, 2014 9:20:58 AM
> >> > Subject: [llvm] r224901 - [X86][ISel] Fix a regression I
> >> > introduced in
> >> > r224884
> >> >
> >> > Author: kfischer
> >> > Date: Sun Dec 28 09:20:57 2014
> >> > New Revision: 224901
> >> >
> >> > URL: http://llvm.org/viewvc/llvm-project?rev=224901&view=rev
> >> > Log:
> >> > [X86][ISel] Fix a regression I introduced in r224884
> >> >
> >> > The else case ResultReg was not checked for validity.
> >> > To my surprise, this case was not hit in any of the
> >> > existing test cases. This includes a new test cases
> >> > that tests this path.
> >> >
> >> > Also drop the `target triple` declaration from the
> >> > original test as suggested by H.J. Lu, because
> >> > apparently with it the test won't be run on Linux
> >>
> >> No, that's not right. All of the llc tests are run, regardless of
> >> the
> >> host; adding a triple is generally done for better test stability
> >> (so that
> 
> Shouldn't it be "target", not "host"? llvm can be configured with
> 
> --target=xxx

That's correct. If you don't provide a triple, llc defaults to calling sys::getDefaultTargetTriple(), which essentially returns LLVM_DEFAULT_TARGET_TRIPLE as set by configure/cmake.

> 
> >> aspects of the host's configuration don't leak into the code
> >> generation for
> >> the test, potentially making the test behave differently on
> >> different hosts.
> >> If there is any chance that the code generation could be sensitive
> >> to
> >> features of the host's triple in some way relevant to what the
> >> test is
> >> checking, you should definitely have a triple. Otherwise, omitting
> >> it is
> >> fine.
> 
> That is true.  That is why I asked if the test was target specific
> and my understanding is it was not specific to Darwin.

Okay, so we're on the same page. I just wanted to make sure that everyone understood what was going on.

 -Hal

> 
> H.J.
> >>
> >>  -Hal
> >>
> >> >
> >> > Added:
> >> >     llvm/trunk/test/CodeGen/X86/large-code-model-isel.ll
> >> > Modified:
> >> >     llvm/trunk/lib/Target/X86/X86FastISel.cpp
> >> >     llvm/trunk/test/CodeGen/X86/fast-isel-call-bool.ll
> >> >
> >> > Modified: llvm/trunk/lib/Target/X86/X86FastISel.cpp
> >> > URL:
> >> >
> >> > http://llvm.org/viewvc/llvm-project/llvm/trunk/lib/Target/X86/X86FastISel.cpp?rev=224901&r1=224900&r2=224901&view=diff
> >> >
> >> > ==============================================================================
> >> > --- llvm/trunk/lib/Target/X86/X86FastISel.cpp (original)
> >> > +++ llvm/trunk/lib/Target/X86/X86FastISel.cpp Sun Dec 28
> >> > 09:20:57
> >> > 2014
> >> > @@ -2709,15 +2709,15 @@ bool X86FastISel::fastLowerCall(CallLowe
> >> >
> >> >        ResultReg =
> >> >          fastEmit_ri(VT, VT, ISD::AND, ResultReg,
> >> >          hasTrivialKill(PrevVal), 1);
> >> > -
> >> > -      if (!ResultReg)
> >> > -        return false;
> >> >      } else {
> >> >        if (!isTypeLegal(Val->getType(), VT))
> >> >          return false;
> >> >        ResultReg = getRegForValue(Val);
> >> >      }
> >> >
> >> > +    if (!ResultReg)
> >> > +      return false;
> >> > +
> >> >      ArgRegs.push_back(ResultReg);
> >> >      OutVTs.push_back(VT);
> >> >    }
> >> >
> >> > Modified: llvm/trunk/test/CodeGen/X86/fast-isel-call-bool.ll
> >> > URL:
> >> >
> >> > http://llvm.org/viewvc/llvm-project/llvm/trunk/test/CodeGen/X86/fast-isel-call-bool.ll?rev=224901&r1=224900&r2=224901&view=diff
> >> >
> >> > ==============================================================================
> >> > --- llvm/trunk/test/CodeGen/X86/fast-isel-call-bool.ll
> >> > (original)
> >> > +++ llvm/trunk/test/CodeGen/X86/fast-isel-call-bool.ll Sun Dec
> >> > 28
> >> > 09:20:57 2014
> >> > @@ -1,8 +1,7 @@
> >> > -; RUN: llc < %s -fast-isel -mcpu=core2 -O1 | FileCheck %s
> >> > +; RUN: llc < %s -fast-isel -mcpu=core2 -march=x86-64 -O1 |
> >> > FileCheck
> >> > %s
> >> >  ; See PR21557
> >> >
> >> >  target datalayout = "e-m:e-i64:64-f80:128-n8:16:32:64-S128"
> >> > -target triple = "x86_64-apple-darwin14.0.0"
> >> >
> >> >  declare i64 @bar(i1)
> >> >
> >> >
> >> > Added: llvm/trunk/test/CodeGen/X86/large-code-model-isel.ll
> >> > URL:
> >> >
> >> > http://llvm.org/viewvc/llvm-project/llvm/trunk/test/CodeGen/X86/large-code-model-isel.ll?rev=224901&view=auto
> >> >
> >> > ==============================================================================
> >> > --- llvm/trunk/test/CodeGen/X86/large-code-model-isel.ll (added)
> >> > +++ llvm/trunk/test/CodeGen/X86/large-code-model-isel.ll Sun Dec
> >> > 28
> >> > 09:20:57 2014
> >> > @@ -0,0 +1,13 @@
> >> > +; RUN: llc < %s -code-model=large -mcpu=core2 -march=x86-64 -O0
> >> > |
> >> > FileCheck %s
> >> > +
> >> > +target datalayout = "e-m:e-i64:64-f80:128-n8:16:32:64-S128"
> >> > +
> >> > + at .str10 = external unnamed_addr constant [2 x i8], align 1
> >> > +
> >> > +define void @foo() {
> >> > +; CHECK-LABEL: foo:
> >> > +entry:
> >> > +; CHECK: callq
> >> > +  %call = call i64* undef(i64* undef, i8* getelementptr
> >> > inbounds ([2
> >> > x i8]* @.str10, i32 0, i32 0))
> >> > +  ret void
> >> > +}
> >> >
> >> >
> >> > _______________________________________________
> >> > llvm-commits mailing list
> >> > llvm-commits at cs.uiuc.edu
> >> > http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits
> >> >
> >>
> >> --
> >> Hal Finkel
> >> Assistant Computational Scientist
> >> Leadership Computing Facility
> >> Argonne National Laboratory
> >
> >
> 
> 
> 
> --
> H.J.
> 

-- 
Hal Finkel
Assistant Computational Scientist
Leadership Computing Facility
Argonne National Laboratory