Page MenuHomePhabricator

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

Authored by hvdijk on Thu, Nov 12, 5:17 AM.

Details

Summary

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

Diff Detail

Event Timeline

hvdijk created this revision.Thu, Nov 12, 5:17 AM
hvdijk requested review of this revision.Thu, Nov 12, 5:17 AM

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 retitled this revision from [X86] Include %rip for 32-bit PIC-relative relocs to [X86] Include %rip for 32-bit RIP-relative relocs for x32.Fri, Nov 13, 12:32 AM
hvdijk edited the summary of this revision. (Show Details)

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 accepted this revision.Fri, Nov 13, 9:36 AM

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

This revision is now accepted and ready to land.Fri, Nov 13, 9:36 AM

LGTM - @hvdijk do you have commit access yet?

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 <harald@gigawatt.nl>". Otherwise I'm pretty sure I can pester someone about it after the weekend :)

This revision was landed with ongoing or failed builds.Sat, Nov 21, 9:20 AM
This revision was automatically updated to reflect the committed changes.
RKSimon reopened this revision.Sun, Nov 22, 4:59 AM

@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.
This revision is now accepted and ready to land.Sun, Nov 22, 4:59 AM

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

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.

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

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 added inline comments.Sun, Nov 22, 4:45 PM
llvm/test/CodeGen/X86/pic.ll
25

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

hvdijk added inline comments.Sun, Nov 22, 5:03 PM
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 added inline comments.Sun, Nov 22, 5:34 PM
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 added inline comments.Sun, Nov 22, 5:50 PM
llvm/test/CodeGen/X86/pic.ll
25

Sure, I've added it there.