[Analysis] Attribute Range should not prevent tail call optimization

[jsji]

• Remove Range attr when comparing for tailcall
• Add test for testcall with range

[llvmbot]

@llvm/pr-subscribers-llvm-selectiondag

@llvm/pr-subscribers-backend-x86

Author: Jinsong Ji (jsji)

Changes

• Remove Range attr when comparing for tailcall
• Add test for testcall with range

---

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

2 Files Affected:

• (modified) llvm/lib/CodeGen/Analysis.cpp (+4-3)
• (added) llvm/test/CodeGen/X86/tailcall-range.ll (+23)

diff --git a/llvm/lib/CodeGen/Analysis.cpp b/llvm/lib/CodeGen/Analysis.cpp
index af7643d93591f7..e693cdbd0ccc1c 100644
--- a/llvm/lib/CodeGen/Analysis.cpp
+++ b/llvm/lib/CodeGen/Analysis.cpp
@@ -593,9 +593,10 @@ bool llvm::attributesPermitTailCall(const Function *F, const Instruction *I,
 
   // Following attributes are completely benign as far as calling convention
   // goes, they shouldn't affect whether the call is a tail call.
-  for (const auto &Attr : {Attribute::Alignment, Attribute::Dereferenceable,
-                           Attribute::DereferenceableOrNull, Attribute::NoAlias,
-                           Attribute::NonNull, Attribute::NoUndef}) {
+  for (const auto &Attr :
+       {Attribute::Alignment, Attribute::Dereferenceable,
+        Attribute::DereferenceableOrNull, Attribute::NoAlias,
+        Attribute::NonNull, Attribute::NoUndef, Attribute::Range}) {
     CallerAttrs.removeAttribute(Attr);
     CalleeAttrs.removeAttribute(Attr);
   }
diff --git a/llvm/test/CodeGen/X86/tailcall-range.ll b/llvm/test/CodeGen/X86/tailcall-range.ll
new file mode 100644
index 00000000000000..1f7712d2494326
--- /dev/null
+++ b/llvm/test/CodeGen/X86/tailcall-range.ll
@@ -0,0 +1,23 @@
+; NOTE: Assertions have been autogenerated by utils/update_llc_test_checks.py UTC_ARGS: --version 4
+; RUN: opt -passes=ipsccp -S -mtriple=x86_64-linux < %s | llc -mtriple=x86_64-linux | FileCheck %s
+
+define i32 @foo(ptr %this, ...) {
+; CHECK-LABEL: foo:
+; CHECK:       # %bb.0: # %entry
+; CHECK-NEXT:    movzbl 0, %eax
+; CHECK-NEXT:    retq
+entry:
+  %call = load volatile i1, ptr null, align 1
+  %spec.select = zext i1 %call to i32
+  ret i32 %spec.select
+}
+
+define i32 @bar(ptr %this, ...) {
+; CHECK-LABEL: bar:
+; CHECK:       # %bb.0: # %entry
+; CHECK-NEXT:    xorl %edi, %edi
+; CHECK-NEXT:    jmp foo@PLT # TAILCALL
+entry:
+  %4 = musttail call i32 (ptr, ...) @foo(ptr null, ...)
+  ret i32 %4
+}

[andjo403]

Do not know so mush about how this works (new contributor to llvm) but it sounds on the comment as this attribute also shall be in the list. There also is the same list of attributes in

llvm-project/llvm/lib/CodeGen/SelectionDAG/TargetLowering.cpp

Line 65 in 1343e68

for (const auto &Attr : {Attribute::Alignment, Attribute::Dereferenceable,

so think that it shall be fixed there also.

[nikic]

Code change looks fine, but the test should not be calling ipsccp. Adjust the input IR to have the range attribute instead.

[andjo403]

shall there not be a update of some test for the TargetLowering.cpp update now that it is added to this PR

[jsji]

shall there not be a update of some test for the TargetLowering.cpp update now that it is added to this PR

Remove the changes from this PR first for now. Will follow up with another PR once I get the test for it ready.

[nikic]

shall there not be a update of some test for the TargetLowering.cpp update now that it is added to this PR

Remove the changes from this PR first for now. Will follow up with another PR once I get the test for it ready.

I'd rather not have these two pieces of code go out of sync.

[jsji]

shall there not be a update of some test for the TargetLowering.cpp update now that it is added to this PR

Remove the changes from this PR first for now. Will follow up with another PR once I get the test for it ready.

I'd rather not have these two pieces of code go out of sync.

I am not able to come up with a reasonable testcase for now.

1. Most of the tests that hit this logic are floating point libcalls.
2. For some libcall returning int, adding range attr won't cause problem

declare range(i64 0, 2) i64 @llvm.llround.f32(float) nounwind readnone
define i64 @testmsxs(float %x) {
  entry:
    %0 = musttail call i64 @llvm.llround.f32(float %x)
      ret i64 %0
}

Let me know if you can find some testcase that shows problems .

[arsenm]

declare range(i64 0, 2) i64 @llvm.llround.f32(float) nounwind readnone
define i64 @testmsxs(float %x) {
entry:
%0 = musttail call i64 @llvm.llround.f32(float %x)
ret i64 %0
}

I'm surprised we allow musttail on intrinsic calls, which aren't really supposed to have ABI

[jsji]

declare range(i64 0, 2) i64 @llvm.llround.f32(float) nounwind readnone
define i64 @testmsxs(float %x) {
entry:
%0 = musttail call i64 @llvm.llround.f32(float %x)
ret i64 %0
}

I'm surprised we allow musttail on intrinsic calls, which aren't really supposed to have ABI

#91347 Opened to track follow up. Thanks.

[jsji]

@nikic @andjo403 Please have a look again. Thanks.

[arsenm]

lgtm with some test nits

[andjo403]

hmm did a comment on the thread of comments on the test but had not noticed that it was marked as resolved do not know if it was removed
I suggested

declare i32 @callee()

define range(i32 0, 2) i32 @func_with_range_attr() {
  %1 = musttail call i32 @callee()
  ret i32 %1
}

define i32 @call_with_range_attr() {
  %1 = musttail call range(i32 0, 2) i32 @callee()
  ret i32 %1
}

[nikic]

LGTM

[nikic]

Suggested change

	%call = load volatile i1, ptr null, align 1
	%call = load volatile i1, ptr %this, align 1

was possibly intended here? Best to avoid "obviously UB" inputs when we can...