Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[X86] Respect blockaddress offsets when performing X86 LEA fixups #71641

Merged
merged 1 commit into from
Nov 10, 2023

Conversation

momo5502
Copy link
Contributor

@momo5502 momo5502 commented Nov 8, 2023

The X86FixupLEAs pass drops blockaddress offsets, when splitting up slow 3-ops LEAs, as can be seen in this example:

https://godbolt.org/z/bEsc3Poje

Before running the pass, the first instruction in bb.0 is a LEA with ebp, ebx and a blockaddress.
After the transformation, the blockaddress is missing.

The reason this happens is because the 3-ops LEA is being splitup into a 2-ops LEA + an add instruction.
However, as hasLEAOffset does not take blockaddresses into consideration, the add is not emitted and thus leading to the offset being dropped.

Taking blockaddresses into consideration fixes this issue and results in the add instruction being emitted.

This fixes #71667

The X86FixupLEAs pass drops blockaddress offsets, when splitting up
slow 3-ops LEAs, as can be seen in this example:

https://godbolt.org/z/bEsc3Poje

Before running the pass, the first instruction in bb.0 is a
LEA with ebp, ebx and a blockaddress.
After the transformation, the blockaddress is missing.

The reason this happens is because the 3-ops LEA is being splitup into
a 2-ops LEA + an add instruction.
However, as hasLEAOffset does not take blockaddresses into consideration,
the add is not emitted and thus leading to the offset being dropped.

Taking blockaddresses into consideration fixes this issue and results
in the add instruction being emitted.
@llvmbot
Copy link
Collaborator

llvmbot commented Nov 8, 2023

@llvm/pr-subscribers-backend-x86

Author: Maurice Heumann (momo5502)

Changes

The X86FixupLEAs pass drops blockaddress offsets, when splitting up slow 3-ops LEAs, as can be seen in this example:

https://godbolt.org/z/bEsc3Poje

Before running the pass, the first instruction in bb.0 is a LEA with ebp, ebx and a blockaddress.
After the transformation, the blockaddress is missing.

The reason this happens is because the 3-ops LEA is being splitup into a 2-ops LEA + an add instruction.
However, as hasLEAOffset does not take blockaddresses into consideration, the add is not emitted and thus leading to the offset being dropped.

Taking blockaddresses into consideration fixes this issue and results in the add instruction being emitted.


Full diff: https://github.com/llvm/llvm-project/pull/71641.diff

2 Files Affected:

  • (modified) llvm/lib/Target/X86/X86FixupLEAs.cpp (+2-1)
  • (added) llvm/test/CodeGen/X86/lea-fixup-blockaddress.mir (+30)
diff --git a/llvm/lib/Target/X86/X86FixupLEAs.cpp b/llvm/lib/Target/X86/X86FixupLEAs.cpp
index ceafbf177352a87..beeebf42dfe81ab 100644
--- a/llvm/lib/Target/X86/X86FixupLEAs.cpp
+++ b/llvm/lib/Target/X86/X86FixupLEAs.cpp
@@ -341,7 +341,8 @@ static inline bool hasInefficientLEABaseReg(const MachineOperand &Base,
 }
 
 static inline bool hasLEAOffset(const MachineOperand &Offset) {
-  return (Offset.isImm() && Offset.getImm() != 0) || Offset.isGlobal();
+  return (Offset.isImm() && Offset.getImm() != 0) || Offset.isGlobal() ||
+         Offset.isBlockAddress();
 }
 
 static inline unsigned getADDrrFromLEA(unsigned LEAOpcode) {
diff --git a/llvm/test/CodeGen/X86/lea-fixup-blockaddress.mir b/llvm/test/CodeGen/X86/lea-fixup-blockaddress.mir
new file mode 100644
index 000000000000000..14134e268613bad
--- /dev/null
+++ b/llvm/test/CodeGen/X86/lea-fixup-blockaddress.mir
@@ -0,0 +1,30 @@
+# NOTE: Assertions have been autogenerated by utils/update_mir_test_checks.py UTC_ARGS: --version 3
+# RUN: llc -mtriple=i386-unknown-linux-gnu -mattr=slow-3ops-lea -run-pass x86-fixup-LEAs -o -  %s | FileCheck %s
+
+--- |
+  define i32 @square(i32 %0) local_unnamed_addr {
+    %blub = getelementptr i8, ptr blockaddress(@square, %2), i32 %0
+    indirectbr ptr %blub, [label %2]
+
+  2:
+    ret i32 0
+  }
+
+---
+name:            square
+body:             |
+  ; CHECK-LABEL: name: square
+  ; CHECK: bb.0:
+  ; CHECK-NEXT:   renamable $eax = LEA32r renamable $ebx, 1, renamable $ebp, 0, $noreg
+  ; CHECK-NEXT:   $eax = ADD32ri $eax, target-flags(x86-gotoff) blockaddress(@square, %ir-block.1), implicit-def $eflags
+  ; CHECK-NEXT:   JMP32r killed renamable $eax
+  ; CHECK-NEXT: {{  $}}
+  ; CHECK-NEXT: bb.1 (%ir-block.1, ir-block-address-taken %ir-block.1):
+  ; CHECK-NEXT:   RET 0
+  bb.0:
+    renamable $eax = LEA32r renamable $ebp, 1, renamable $ebx, target-flags(x86-gotoff) blockaddress(@square, %ir-block.1), $noreg
+    JMP32r killed renamable $eax
+
+  bb.1 (%ir-block.1, ir-block-address-taken %ir-block.1):
+    RET 0
+...

@momo5502
Copy link
Contributor Author

momo5502 commented Nov 8, 2023

I tried to minimize my test case as much as possible. Now, the IR code does not really match the MIR code anymore.
However, I was not able to remove it, as the blockaddress in MIR has a reference to an IR BB. I'm sure there is a way to rewrite that, but I didn't know how :D

@RKSimon
Copy link
Collaborator

RKSimon commented Nov 8, 2023

Do we have an Issue# to track this?

@momo5502
Copy link
Contributor Author

momo5502 commented Nov 8, 2023

Do we have an Issue# to track this?

I don't think there is one and I have not created one myself. Should I do so?

@phoebewang
Copy link
Contributor

I tried to minimize my test case as much as possible. Now, the IR code does not really match the MIR code anymore. However, I was not able to remove it, as the blockaddress in MIR has a reference to an IR BB. I'm sure there is a way to rewrite that, but I didn't know how :D

I don't think there is one and I have not created one myself. Should I do so?

I think it would be good to create an issue and comment the IR cannot demonstrate the issue by itself in the test case.

@momo5502
Copy link
Contributor Author

momo5502 commented Nov 8, 2023

I created #71667

Copy link
Contributor

@phoebewang phoebewang left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM.

@momo5502
Copy link
Contributor Author

momo5502 commented Nov 9, 2023

Now that it is approved, would anyone mind merging the PR? I don't have commit access :)

@phoebewang phoebewang merged commit 8cbfc0b into llvm:main Nov 10, 2023
4 checks passed
zahiraam pushed a commit to zahiraam/llvm-project that referenced this pull request Nov 20, 2023
…vm#71641)

The X86FixupLEAs pass drops blockaddress offsets, when splitting up slow
3-ops LEAs, as can be seen in this example:

https://godbolt.org/z/bEsc3Poje

Before running the pass, the first instruction in bb.0 is a LEA with
ebp, ebx and a blockaddress.
After the transformation, the blockaddress is missing.

The reason this happens is because the 3-ops LEA is being splitup into a
2-ops LEA + an add instruction.
However, as hasLEAOffset does not take blockaddresses into
consideration, the add is not emitted and thus leading to the offset
being dropped.

Taking blockaddresses into consideration fixes this issue and results in
the add instruction being emitted.

This fixes llvm#71667
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[X86] Blockaddress offsets are lost when performing X86 LEA fixups with slow 3-ops LEAs
4 participants