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.

dstenb added a subscriber: dstenb.Thu, Nov 26, 1:13 AM

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

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.