Fix the Thumb test for vfloat intrinsics

[pbarrio]

Summary

This test was not testing the intrinsics. A function like this:

define %v4f32 @test_v4f32.floor(%v4f32 %a){
...

%1 = call %v4f32 @llvm.floor.v4f32(%v4f32 %a)

...
}

is transformed into the following assembly:

_test_v4f32.floor: @ @test_v4f32.floor
...

bl _floorf

...

In each function tested, there are two CHECK: one that checked
for the label and another one for the intrinsic that should be used
inside the function (in our case, "floor"). However, although the
first CHECK was matching the label, the second was not matching the
intrinsic, but the second "floor" in the same line as the label.

This is fixed by making the first CHECK match the entire line.

[ARM] Add ".code 32" to functions in the ARM instruction set

Before, only Thumb functions were marked as ".code 16". These
".code x" directives are effective until the next directive of its
kind is encountered. Therefore, in code with interleaved ARM and
Thumb functions, it was possible to declare a function as ARM and
end up with a Thumb function after assembly. A test has been added.

An existing test has also been fixed to take this change into
account.

[jmolloy]

Hi Pablo,

There are two changes here, and they should have been submitted separately and not in one phab review.

That said, they look simple enough to disentangle so I've reviewed them. The .code32 one LGTM. I have a query about the other.

When submitting these, please ensure they are submitted as two separate commits.

James

test/CodeGen/ARM/vfloatintrinsics.ll

322 ↗

(On Diff #70679)

Why has this testcase been removed?

[pbarrio]

Ah, sorry. They are two different changes in my tree, and I thought arcanist would preserve that but it didn't. I will make sure that they are submitted as separate patches.

test/CodeGen/ARM/vfloatintrinsics.ll

322 ↗

(On Diff #70679)

This testcase was failing after my changes: the currenty generated assembly is not calling an fabs intrinsic but a series of bfc and vmovs.

I am not sure what the right checks would be, so I have removed this part of the test (as it's not currently testing anything useful anyway). I have added aschwaighofer as a reviewer, who is the original author, as he may know what the correct test would be.

[jmolloy]

That'll be because it's clearing the MSB of the floating point value in the integer domain:

fabs(float f) = ((int)x & ~(1U<<31))

LGTM.

[rengolin]

Hi Pablo,

The test match is an unfortunate side-effect of regular expressions and how the CHECK lines behave, sounds like a good fix, but you shouldn't delete the test, as James said.

Regarding the ".code 32", I agree this is a problem, but I'm unsure what's the solution. Right now, since the default is ARM, we only emit when it's not. This is an emulation of the original GNU behaviour and, unless we have a good reason not to, we should be following the same pattern, especially for assembly code, which is a bit irrelevant in the LLVM world.

We also have that problem for all other directives that leak meaning (.align, .arch, .cpu, .fpu, .arch_extension, etc). But adding all of them to the beginning of each function is not the way to go. We're discussing no the side a way to add push/pop to those directives, at least supported in the LLVM assembler, and this could help this case.

Is this change at all related to the tests? If not, I suggest you refrain from submitting it until we can form a consensus. It may look like a simple matter, but backwards compatibility in this case can lead to invisible code generation failures.

cheers,
--renato

[pbarrio]

The changes to the vfloatintrinsics tests are independent from the functional change to add ".code 32", although I discovered them while working on this bug. I will close this review, and create a separate one with the test changes only.

What you say about .code directives makes sense to me, specially about making sure that the assembler interprets the directives correctly. I anticipated some discussion but I didn't know there was already a conversation about this.

[rengolin]

In D24337#537289, @pbarrio wrote:

What you say about .code directives makes sense to me, specially about making sure that the assembler interprets the directives correctly. I anticipated some discussion but I didn't know there was already a conversation about this.

There has been a loooong chat (many years) about GNU emulation and how to work around its broken legacy requirements without destroying existing code.

We should do this over beer, though... as it's as boring as it is long... :)

[pbarrio]

On second thoughts, I have tested with a modern gcc and it marks all functions with either .thumb or .arm (equivalent to .code 16/32). So, essentially, that behaviour is what this patch suggests. Another thing that surprised me is that Thumb appears to be the default, contrary to the LLVM default which is ARM.

When you talk about preserving the "original GNU" behaviour, do you mean an old GNU toolchain? If so, why should we preserve an ancient behaviour?

Sorry if I'm missing something or this is the N-th time it is discussed on the list. If you can point me to older discussions it would be great :)

[rengolin]

In D24337#538293, @pbarrio wrote:

On second thoughts, I have tested with a modern gcc and it marks all functions with either .thumb or .arm (equivalent to .code 16/32). So, essentially, that behaviour is what this patch suggests. Another thing that surprised me is that Thumb appears to be the default, contrary to the LLVM default which is ARM.

Oh, there it is, GCC 6 does it. You are right!

When you talk about preserving the "original GNU" behaviour, do you mean an old GNU toolchain? If so, why should we preserve an ancient behaviour?

I surely am, and I agree with you it should be fine.

Even if we still don't have a solution for all the other directives, ARM/Thumb is probably more critical than any other.

Please, split this patch with just the .code and re-submit.

Sorry for the noise.

--renato

[pbarrio]

Removed vector float intrinsic test from this review. That
part of the patch has now been committed and the review can
be followed in https://reviews.llvm.org/D24398

This is now only the fix to add .code 32 to ARM functions
and its corresponding test.

[pbarrio]

Renato, James, thank you very much for the review.

My understanding is that the patch has been approved? I will commit later today, but I will leave some time so that you can comment on the update to the patch.

Thanks,
Pablo

[rengolin]

Yes, sorry, I was waiting for you to update the patch and got lost in the noise. LGTM.

Thanks!

llvm/trunk/lib/Target/ARM/ARMAsmPrinter.cpp

void ARMAsmPrinter::EmitFunctionBodyEnd() {
   InConstantPool = false;
   OutStreamer->EmitDataRegion(MCDR_DataRegionEnd);
 }
 
 void ARMAsmPrinter::EmitFunctionEntryLabel() {
   if (AFI->isThumbFunction()) {
     OutStreamer->EmitAssemblerFlag(MCAF_Code16);
     OutStreamer->EmitThumbFunc(CurrentFnSym);
+  } else {
+    OutStreamer->EmitAssemblerFlag(MCAF_Code32);
   }
-
   OutStreamer->EmitLabel(CurrentFnSym);
 }
 
 void ARMAsmPrinter::EmitXXStructor(const DataLayout &DL, const Constant *CV) {
   uint64_t Size = getDataLayout().getTypeAllocSize(CV->getType());
   assert(Size && "C++ constructor pointer had zero size!");
 
   const GlobalValue *GV = dyn_cast<GlobalValue>(CV->stripPointerCasts());

llvm/trunk/test/CodeGen/ARM/interwork.ll

+; One file may have multiple functions targeted at different (ARM, Thumb)
+; instruction sets. Passing this information to the linker and the assembler
+; is done through the ".code 16" and ".code 32" directives.
+;
+; RUN: llc -mtriple=arm-arm-none-eabi %s -o - | FileCheck %s
+
+define void @ft() #0 {
+; CHECK: .code 16
+; CHECK: .thumb_func
+; CHECK-LABEL: ft:
+entry:
+  ret void
+}
+
+define void @fz() #1 {
+; CHECK: .code 32
+; CHECK-LABEL: fz:
+entry:
+  ret void
+}
+
+attributes #0 = { "target-features"="+thumb-mode" }
+attributes #1 = { "target-features"="-thumb-mode" }

llvm/trunk/test/DebugInfo/ARM/header.ll

 ; RUN: llc -mtriple armv7-apple-darwin < %s  | FileCheck %s
 
 ; Test that we don't pollute the start of the file with debug sections.
 ; This is particularly important on ARM MachO as a change in section order can
 ; cause a change the relaxation of the instructions used.
 
 ; CHECK:      .section        __TEXT,__text,regular,pure_instructions
 ; CHECK-NEXT: .syntax unified
 ; CHECK-NEXT: .globl  _f
 ; CHECK-NEXT: .p2align  2
-; CHECK-NEXT: _f:                    @ @f
+; CHECK-NEXT: .code 32                    @ @f
+; CHECK-NEXT: _f:
 
 ; CHECK:  .section        __DWARF,__debug_str,regular,debug
 
 define void @f() !dbg !4 {
   ret void, !dbg !9
 }
 !llvm.dbg.cu = !{!0}
 !llvm.module.flags = !{!7, !8}
 
 !0 = distinct !DICompileUnit(language: DW_LANG_C99, file: !1, producer: "foo", isOptimized: true, runtimeVersion: 0, emissionKind: FullDebug, enums: !2, retainedTypes: !2, globals: !2, imports: !2)
 !1 = !DIFile(filename: "/foo/test.c", directory: "/foo")
 !2 = !{}
-!4 = distinct !DISubprogram(name: "f", scope: !1, file: !1, line: 1, type: !5, isLocal: false, isDefinition: true, scopeLine: 1, flags: DIFlagPrototyped, isOptimized: true, unit: !0, variables: !2)
+!4 = distinct !DISubprogram(name: "f", scope: !1, file: !1, line: 2, type: !5, isLocal: false, isDefinition: true, scopeLine: 1, flags: DIFlagPrototyped, isOptimized: true, unit: !0, variables: !2)
 !5 = !DISubroutineType(types: !6)
 !6 = !{null}
 !7 = !{i32 2, !"Dwarf Version", i32 4}
 !8 = !{i32 2, !"Debug Info Version", i32 3}
 !9 = !DILocation(line: 1, column: 15, scope: !4)