llvm-project icon indicating copy to clipboard operation
llvm-project copied to clipboard

missed fold, sext(fptosi x to i32) => fptosi x to i64

Open zhengyang92 opened this issue 11 months ago • 7 comments

https://alive2.llvm.org/ce/z/bFW9vZ online Alive timed out for this one, you'll need local alive2 with larger smt-to.

define i64 @src(float %0) {
if.end27:
%1 = fptosi float %0 to i32
%2 = sext i32 %1 to i64
ret i64 %2

sink: ; No predecessors!
unreachable
}

define i64 @tgt(float %0) {
if.end27:
%1 = fptosi float %0 to i64
ret i64 %1

sink: ; No predecessors!
unreachable
}

no brainer, same pattern applies to fptoui.

zhengyang92 avatar Mar 14 '24 17:03 zhengyang92

@jayfoad @regehr

zhengyang92 avatar Mar 14 '24 17:03 zhengyang92

Hi!

This issue may be a good introductory issue for people new to working on LLVM. If you would like to work on this issue, your first steps are:

  1. In the comments of the issue, request for it to be assigned to you.
  2. Fix the issue locally.
  3. Run the test suite locally. Remember that the subdirectories under test/ create fine-grained testing targets, so you can e.g. use make check-clang-ast to only run Clang's AST tests.
  4. Create a Git commit.
  5. Run git clang-format HEAD~1 to format your changes.
  6. Open a pull request to the upstream repository on GitHub. Detailed instructions can be found in GitHub's documentation.

If you have any further questions about this issue, don't hesitate to ask via a comment in the thread below.

llvmbot avatar Mar 14 '24 17:03 llvmbot

@llvm/issue-subscribers-good-first-issue

Author: Zhengyang Liu (zhengyang92)

https://alive2.llvm.org/ce/z/bFW9vZ online Alive timed out for this one, you'll need local alive2 with larger smt-to.
define i64 @<!-- -->src(float %0) {
if.end27:
%1 = fptosi float %0 to i32
%2 = sext i32 %1 to i64
ret i64 %2

sink: ; No predecessors!
unreachable
}

define i64 @<!-- -->tgt(float %0) {
if.end27:
%1 = fptosi float %0 to i64
ret i64 %1

sink: ; No predecessors!
unreachable
}

no brainer, same pattern applies to fptoui.

llvmbot avatar Mar 14 '24 17:03 llvmbot

My feeling is that we probably should not do this in IR because:

  • It will not be a win on all targets. Float-to-i64 could be significantly more expensive than float-to-i32 on targets that do not have a single instruction for it (whereas sext i32 to i64 is a single fast instruction on every target we care about).
  • Once you've done this transform, it's hard to undo it later.

Instead it could be done as a target-specific DAG combine or instruction selection pattern.

jayfoad avatar Mar 14 '24 18:03 jayfoad

I agree that we should not do this in IR.

nikic avatar Mar 14 '24 21:03 nikic

@nikic @jayfoad agreed.

zhengyang92 avatar Mar 14 '24 21:03 zhengyang92

I'd like to work on this issue if possible 😀

miguelraz avatar Mar 24 '24 14:03 miguelraz

@miguelraz Are you still planning to work on this? Otherwise I'd like pick it up.

HendrikHuebner avatar Apr 21 '24 22:04 HendrikHuebner