LLVM gen correct asm info for mingw and cygwin arm targets

[martell]

Summary

Relative clang review

http://reviews.llvm.org/D11071

[yaron.keren]

Hi, could you update both patches with more context as detailed in

http://llvm.org/docs/Phabricator.html

it really makes reviewing and discussing much easier with the context around the patches.

[martell]

In D11075#202080, @yaron.keren wrote:

Hi, could you update both patches with more context as detailed in

http://llvm.org/docs/Phabricator.html

it really makes reviewing and discussing much easier with the context around the patches.

done :)

[t.p.northover]

It should be possible to write tests for this; we try to make sure all patches with functional changes have something committed under tests/ too, to prevent mistakes on later refactoring if nothing else.

Tim.

[martell]

In D11075#202358, @t.p.northover wrote:

It should be possible to write tests for this; we try to make sure all patches with functional changes have something committed under tests/ too, to prevent mistakes on later refactoring if nothing else.

Tim.

The relative tests for this code path have already been introduced in this revision
http://reviews.llvm.org/D3231

I could recreate all the tests with the windows-gnu triplet but they would all fail until
http://reviews.llvm.org/D11071
is approved and merged

Should we wait for that ?

[t.p.northover]

I could recreate all the tests with the windows-gnu triplet but they would all fail until http://reviews.llvm.org/D11071 is approved and merged

Speaking of which, that could do with some tests too.

But I'm not sure how a Clang patch can have any influence on LLVM tests. This appears to be changing something about how llc would emit object files (and assembly? I don't know) when using these triples. That's what should be tested with this patch.

I could recreate all the tests with the windows-gnu triplet

If the same behaviour is expected, you should be able to just add a second "RUN" line to those tests.

Cheers.

Tim.

[martell]

Added relevant test cases as requested by Tim :)

[t.p.northover]

I'm afraid I don't think these are the right tests to be adding.

They all seem to pass already, so they're unrelated to the change you're making. The only difference I've seen during some brief fiddling is when -filetype=obj is used (your change prevents some assertion failure), though looking at the source it probably affects some other details too.

[rnk]

lgtm with those tests

lib/Target/ARM/MCTargetDesc/ARMMCTargetDesc.cpp

281–285

It's probably cleaner to flip the order to this:

else if (TheTriple.isWindowsMSVCEnvironment())
  MAI = new ARMCOFFMCAsmInfoMicrosoft();
else if (TheTriple.isOSWindows())
  MAI = new ARMCOFFMCAsmInfoGNU();
else ...

We basically have the MS environment and then the GNU-ish "everything else" one.

test/CodeGen/ARM/Windows/no-arm-mode.ll

9

OK, so mingw will presumably also be focusing on a thumb-only, winrt, environment?

test/CodeGen/ARM/Windows/pic.ll

6

Can you add a comment about the purpose of this test? Is the code sequence below actually a PIC sequence?

[martell]

In D11075#202824, @t.p.northover wrote:

I'm afraid I don't think these are the right tests to be adding.

They all seem to pass already, so they're unrelated to the change you're making. The only difference I've seen during some brief fiddling is when -filetype=obj is used (your change prevents some assertion failure), though looking at the source it probably affects some other details too.

Yes you are correct it does prevent an assertion failure.
Itanium seems to use these tests for that purpose also so I don't think I should make a specific assertion test for gnu when there isn't one for itanium or msvc.
The test structors.ll has a different section name with gnu so this test would most definitely fail if the target wasn't supported.

The only other test I can think of todo is alloca.ll where we use chkstk_ms instead of chkstk but I will put that in a separate commit as i have to edit the code gen for that.

In D11075#202858, @rnk wrote:

OK, so mingw will presumably also be focusing on a thumb-only, winrt, environment?

Yes the target will windows NT just like msvc and itanium :)
Windows store / Desktop

Can you add a comment about the purpose of this test? Is the code sequence below actually a PIC sequence?

I could be wrong but to me the purpose of this test is to ensure to load of an external global is done via the movw movt pair
A branch is done the same way and this test seems to ensure the relocation model doesn't affect this.
I was actually not expecting gnu to do this the correct way.

It's probably cleaner to flip the order to this:
else if (TheTriple.isWindowsMSVCEnvironment())

MAI = new ARMCOFFMCAsmInfoMicrosoft();

else if (TheTriple.isOSWindows())

MAI = new ARMCOFFMCAsmInfoGNU();

else ...
We basically have the MS environment and then the GNU-ish "everything else" one.

I will update the patch to reflect this :)

[martell]

updated to a less expensive check as suggested by rnk

[martell]

ping! can someone commit this please :)

[yaron.keren]

r242123

lib/Target/ARM/MCTargetDesc/ARMMCTargetDesc.cpp

static MCRegisterInfo *createARMMCRegisterInfo(const Triple &Triple) {
   InitARMMCRegisterInfo(X, ARM::LR, 0, 0, ARM::PC);
   return X;
 }
 
 static MCAsmInfo *createARMMCAsmInfo(const MCRegisterInfo &MRI,
                                      const Triple &TheTriple) {
   MCAsmInfo *MAI;
   if (TheTriple.isOSDarwin() || TheTriple.isOSBinFormatMachO())
     MAI = new ARMMCAsmInfoDarwin(TheTriple);
-  else if (TheTriple.isWindowsItaniumEnvironment())
-    MAI = new ARMCOFFMCAsmInfoGNU();
   else if (TheTriple.isWindowsMSVCEnvironment())
     MAI = new ARMCOFFMCAsmInfoMicrosoft();
+  else if (TheTriple.isOSWindows())
+    MAI = new ARMCOFFMCAsmInfoGNU();
   else
     MAI = new ARMELFMCAsmInfo(TheTriple);
 
   unsigned Reg = MRI.getDwarfRegNum(ARM::SP, true);
   MAI->addInitialFrameState(MCCFIInstruction::createDefCfa(nullptr, Reg, 0));
 
   return MAI;
 }

test/CodeGen/ARM/Windows/hard-float.ll

-; RUN: llc -mtriple=thumbv7-windows-itanium -mcpu=cortex-a9 -o - %s | FileCheck %s
+; RUN: llc -mtriple=thumbv7-windows-itanium -mcpu=cortex-a9 -o - %s \
+; RUN:   | FileCheck %s -check-prefix CHECK-WIN
+
+; RUN: llc -mtriple=thumbv7-windows-gnu -mcpu=cortex-a9 -o - %s \
+; RUN:   | FileCheck %s -check-prefix CHECK-GNU
 
 define float @function(float %f, float %g) nounwind {
 entry:
   %h = fadd float %f, %g
   ret float %h
 }
 
-; CHECK: vadd.f32 s0, s0, s1
+; CHECK-WIN: vadd.f32 s0, s0, s1
+
+; CHECK-GNU: vadd.f32 s0, s0, s1
 

test/CodeGen/ARM/Windows/no-arm-mode.ll

 ; RUN: not llc -mtriple=armv7-windows-itanium -mcpu=cortex-a9 -o /dev/null %s 2>&1 \
-; RUN:  | FileCheck %s
+; RUN:   | FileCheck %s -check-prefix CHECK-WIN
 
-; CHECK: does not support ARM mode execution
+; RUN: not llc -mtriple=armv7-windows-gnu -mcpu=cortex-a9 -o /dev/null %s 2>&1 \
+; RUN:   | FileCheck %s -check-prefix CHECK-GNU
+
+; CHECK-WIN: does not support ARM mode execution
+
+; CHECK-GNU: does not support ARM mode execution
 

test/CodeGen/ARM/Windows/pic.ll

 ; RUN: llc -mtriple thumbv7-windows-itanium -relocation-model pic -filetype asm -o - %s \
-; RUN:    | FileCheck %s
+; RUN:   | FileCheck %s -check-prefix CHECK-WIN
+
+; RUN: llc -mtriple thumbv7-windows-gnu -relocation-model pic -filetype asm -o - %s \
+; RUN:   | FileCheck %s -check-prefix CHECK-GNU
 
 @external = external global i8
 
 define arm_aapcs_vfpcc i8 @return_external() {
 entry:
   %0 = load i8, i8* @external, align 1
   ret i8 %0
 }
 
-; CHECK-LABEL: return_external
-; CHECK: movw r0, :lower16:external
-; CHECK: movt r0, :upper16:external
-; CHECK: ldrb r0, [r0]
+; CHECK-WIN-LABEL: return_external
+; CHECK-WIN: movw r0, :lower16:external
+; CHECK-WIN: movt r0, :upper16:external
+; CHECK-WIN: ldrb r0, [r0]
 
+; CHECK-GNU-LABEL: return_external
+; CHECK-GNU: movw r0, :lower16:external
+; CHECK-GNU: movt r0, :upper16:external
+; CHECK-GNU: ldrb r0, [r0]

test/CodeGen/ARM/Windows/structors.ll

-; RUN: llc -mtriple thumbv7-windows-itanium -o - %s | FileCheck %s
+; RUN: llc -mtriple thumbv7-windows-itanium -o - %s \
+; RUN:   | FileCheck %s -check-prefix CHECK-WIN
+
+; RUN: llc -mtriple thumbv7-windows-gnu -o - %s \
+; RUN:   | FileCheck %s -check-prefix CHECK-GNU
 
 @llvm.global_ctors = appending global [1 x { i32, void ()*, i8* }] [{ i32, void ()*, i8* } { i32 65535, void ()* @function, i8* null }]
 
 define arm_aapcs_vfpcc void @function() {
 entry:
   ret void
 }
 
-; CHECK: .section .CRT$XCU,"dr"
-; CHECK: .long function
+; CHECK-WIN: .section .CRT$XCU,"dr"
+; CHECK-WIN: .long function
 
+; CHECK-GNU: .section .ctors,"dw"
+; CHECK-GNU: .long function