[X86] Include %rip for 32-bit RIP-relative relocs for x32

[hvdijk]

Summary

%rip was only included for 64-bit RIP-relative relocations, but needs to be included for 32-bit as well.

[MaskRay]

This is correct. Probably worth mentioning in the subject that this is for x32. I wonder whether a fastisel-* test file is more suitable.

[hvdijk]

In D91339#2392968, @MaskRay wrote:

This is correct. Probably worth mentioning in the subject that this is for x32.

Sure, added. I had also wrongly written "PIC-relative" which should clearly be "RIP-relative", edited that as well.

I wonder whether a fastisel-* test file is more suitable.

I had started with a new test, but switched to extending pic.ll instead because it was more thorough. I can copy pic.ll to fastisel-pic.ll and only check x32 fastisel in there, if preferred.

[MaskRay]

In D91339#2393183, @hvdijk wrote:

In D91339#2392968, @MaskRay wrote:

This is correct. Probably worth mentioning in the subject that this is for x32.

Sure, added. I had also wrongly written "PIC-relative" which should clearly be "RIP-relative", edited that as well.

I wonder whether a fastisel-* test file is more suitable.

I had started with a new test, but switched to extending pic.ll instead because it was more thorough. I can copy pic.ll to fastisel-pic.ll and only check x32 fastisel in there, if preferred.

It may be fine. Let's wait a bit for other reviewers

[RKSimon]

LGTM - @hvdijk do you have commit access yet?

[hvdijk]

In D91339#2409588, @RKSimon wrote:

LGTM - @hvdijk do you have commit access yet?

I don't. If you're willing to commit it, I'd appreciate it, author info is "Harald van Dijk <[e-mail]>". Otherwise I'm pretty sure I can pester someone about it after the weekend :)

[RKSimon]

@hvdijk On EXPENSIVE_CHECKS builds I'm seeing a build failure in pic.ll - please can you take a look?

Command Output (stdout):
--
$ ":" "RUN: at line 1"
$ "e:\llvm\ninja\bin\llc.exe" "-mcpu=generic" "-mtriple=i686-pc-linux-gnu" "-relocation-model=pic" "-asm-verbose=false" "-post-RA-scheduler=false"
$ "e:\llvm\ninja\bin\filecheck.exe" "E:\llvm\llvm-project\llvm\test\CodeGen\X86\pic.ll" "-check-prefixes=CHECK,CHECK-I686"
$ ":" "RUN: at line 2"
$ "e:\llvm\ninja\bin\llc.exe" "-mcpu=generic" "-mtriple=x86_64-pc-linux-gnux32" "-relocation-model=pic" "-asm-verbose=false" "-post-RA-scheduler=false"
# command stderr:

# After X86 Fixup SetCC
# Machine code for function test6: IsSSA, TracksLiveness
Constant Pool:
  cp#0: [double 1.234560e+02, double 4.561230e+02], align=8
Function Live Ins: $edi in %0

bb.0.entry:
  liveins: $edi
  %0:gr32 = COPY $edi
  %5:gr32 = MOV32r0 implicit-def $eflags
  TEST32rr %0:gr32, %0:gr32, implicit-def $eflags
  %1:gr8 = SETCCr 4, implicit $eflags
  %6:gr32 = INSERT_SUBREG %5:gr32(tied-def 0), %1:gr8, %subreg.sub_8bit
  %3:gr32 = LEA64_32r $rip, 1, $noreg, %const.0, $noreg
  %4:fr64 = MOVSDrm_alt killed %3:gr32, 8, killed %6:gr32, 0, $noreg :: (load 8 from constant-pool)
  $xmm0 = COPY %4:fr64
  RET 0, $xmm0

# End machine code for function test6.

*** Bad machine code: Illegal virtual register for instruction ***
- function:    test6
- basic block: %bb.0 entry (0x26ed631e9a0)
- instruction: %4:fr64 = MOVSDrm_alt killed %3:gr32, 8, killed %6:gr32, 0, $noreg :: (load 8 from constant-pool)
- operand 3:   killed %6:gr32
Expected a GR32_NOSP register, but got a GR32 register
LLVM ERROR: Found 1 machine code errors.

[hvdijk]

In D91339#2409998, @RKSimon wrote:

@hvdijk On EXPENSIVE_CHECKS builds I'm seeing a build failure in pic.ll - please can you take a look?

Sorry about that, already doing so. The code change in here is correct, I think, but there are two independent pre-existing problems that are exposed by the test when expensive checks are enabled. For one I have opened https://reviews.llvm.org/D91924, I am still looking for the right way to fix the other one.

[hvdijk]

I have now also opened https://reviews.llvm.org/D91933 hoping to fix the other pre-existing issue. With both https://reviews.llvm.org/D91924 and that, the test in here passes even when expensive checks are enabled.

[RKSimon]

Thanks - please can you add -verify-machineinstrs to the pic.ll command lines as part of either of those patches?

[hvdijk]

In D91339#2410165, @RKSimon wrote:

Thanks - please can you add -verify-machineinstrs to the pic.ll command lines as part of either of those patches?

Sure, added to D91933.

[pengfei]

llvm/test/CodeGen/X86/pic.ll

25

Sorry for not finding it in time.
The prefix CHECK-DAG-X32 seems not been used.

[hvdijk]

llvm/test/CodeGen/X86/pic.ll

25

Good spot! I meant CHECK-X32-DAG here, and with that, the test still passes. Should I submit that fix as a standalone diff for new review?

[pengfei]

llvm/test/CodeGen/X86/pic.ll

25

FileCheck doesn't check unused prefix for now. I'm not sure. Maybe you can fix it in the patch you fixing builtbot fail.

[hvdijk]

llvm/test/CodeGen/X86/pic.ll

25

Sure, I've added it there.

[dstenb]

What is the status of the expensive check failure? It has been present for five days now. Should we revert this patch until that is resolved?

(However, I don't know if we are more permissive with failures in the expensive checks jobs than other jobs.)

[hvdijk]

In D91339#2417918, @dstenb wrote:

What is the status of the expensive check failure? It has been present for five days now.

The fixes are ready but have not been approved yet. They are D91924 and D91933.

Should we revert this patch until that is resolved?

The test could be disabled for x32 until those other fixes go in though, if wanted. There should be no need to revert the functional part of this patch: the breakage is unrelated to it, this is merely something that was previously not covered by any tests.

[RKSimon]

@hvdijk It'd be very useful if you got commit access - https://llvm.org/docs/DeveloperPolicy.html#obtaining-commit-access

llvm/test/CodeGen/X86/pic.ll

 ; CHECK-I686: calll .L0$pb
 ; CHECK-I686-NEXT:  .L0$pb:
 ; CHECK-I686-NEXT:  popl
 ; CHECK-I686: addl  $_GLOBAL_OFFSET_TABLE_+(.L{{.*}}-.L0$pb),
 ; CHECK-I686: movl  dst@GOT(%eax),
 ; CHECK-I686: movl  ptr@GOT(%eax),
 ; CHECK-I686: movl  src@GOT(%eax),
 ; CHECK-I686: ret
-; CHECK-DAG-X32:  movl  dst@GOTPCREL(%rip),
-; CHECK-DAG-X32:  movl  ptr@GOTPCREL(%rip),
-; CHECK-DAG-X32:  movl  src@GOTPCREL(%rip),
+; CHECK-X32-DAG:  movl  dst@GOTPCREL(%rip),
+; CHECK-X32-DAG:  movl  ptr@GOTPCREL(%rip),
+; CHECK-X32-DAG:  movl  src@GOTPCREL(%rip),
 ; CHECK-X32:  retq
 }
 
 @ptr2 = global i32* null
 @dst2 = global i32 0
 @src2 = global i32 0
 
 define void @test1() nounwind {
 entry:
     store i32* @dst2, i32** @ptr2
     %tmp.s = load i32, i32* @src2
     store i32 %tmp.s, i32* @dst2
     ret void
     
 ; CHECK-LABEL:  test1:
 ; CHECK-I686: calll .L1$pb
 ; CHECK-I686-NEXT:  .L1$pb:
 ; CHECK-I686-NEXT:  popl
 ; CHECK-I686: addl  $_GLOBAL_OFFSET_TABLE_+(.L{{.*}}-.L1$pb), %eax
 ; CHECK-I686: movl  dst2@GOT(%eax),
 ; CHECK-I686: movl  ptr2@GOT(%eax),
 ; CHECK-I686: movl  src2@GOT(%eax),
 ; CHECK-I686: ret
-; CHECK-DAG-X32:  movl  dst2@GOTPCREL(%rip),
-; CHECK-DAG-X32:  movl  ptr2@GOTPCREL(%rip),
-; CHECK-DAG-X32:  movl  src2@GOTPCREL(%rip),
+; CHECK-X32-DAG:  movl  dst2@GOTPCREL(%rip),
+; CHECK-X32-DAG:  movl  ptr2@GOTPCREL(%rip),
+; CHECK-X32-DAG:  movl  src2@GOTPCREL(%rip),
 ; CHECK-X32:  retq
 
 }
 
 declare i8* @malloc(i32)
 
 define void @test2() nounwind {
 entry: