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

[Hal Finkel]

----- Original Message -----
> From: "Keno Fischer" <kfischer at college.harvard.edu>
> To: "Hal Finkel" <hfinkel at anl.gov>, "H.J. Lu" <hjl.tools at gmail.com>
> Cc: "Commit Messages and Patches for LLVM" <llvm-commits at cs.uiuc.edu>
> Sent: Monday, December 29, 2014 4:41:26 AM
> Subject: Re: [llvm] r224901 - [X86][ISel] Fix a regression I introduced in r224884
> 
> 
> Ok, what's the general rule on whether to include `target triple` and
> `target datalayout` in the tests?

The general rule is essentially:

  If there is any chance that the code generation could be sensitive to features of the default target's triple in some way relevant to what the test is checking, you should definitely have a triple. Otherwise, omitting it is fine.

At this point in time, almost all tests have triples. Predicting what the triple will affect (at the present time and in the future) is just too difficult in almost all non-trivial situations, and not having a triple will often lead to failures that are non-trivial to reproduce locally.

 -Hal

> 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 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.
> 
> -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
> 
> 

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