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

Missing case of compile-time constant folding #547

Closed
tschwinge opened this issue Jul 5, 2021 · 6 comments · Fixed by #673
Closed

Missing case of compile-time constant folding #547

tschwinge opened this issue Jul 5, 2021 · 6 comments · Fixed by #673

Comments

@tschwinge
Copy link
Member

The example code https://godbolt.org/z/8o74c57Yj (via https://lkml.org/lkml/2021/7/4/171, via https://gcc-rust.zulipchat.com/#narrow/stream/266897-general/topic/Rust.20for.20Linux.20RFC.3A.20Another.20mention.20of.20GCC-RS) shows a missing case of compile-time constant folding.

At this time, we produce (see gccrs-snapshot tab):

TestCrate::S::sum:
        lea     rax, [rdi+rsi]
        ret
TestCrate::f:
        mov     edi, 123
        mov     esi, 42
        jmp     TestCrate::S::sum

..., whereas rustc 1.53.0 produces:

example::f:
        mov     eax, 165
        ret
@tschwinge
Copy link
Member Author

(Note that I haven't checked what generally we're doing with respect to compile-time constant folding.)

@philberty
Copy link
Member

philberty commented Jul 5, 2021

I think the bug isn't necessarily to do with constant folding in the front-end. I think it is because of the missing: #241

Currently when we define a method with &self the reference part of the type is currently ignored since we need to support #434 as part of that in the type system. Its something I was going to be working on this month. I was able to verify this by testing a similar c++ piece of code that GCC will optimize this but from that example in rust our gimple dump shows:

usize S_sum (const struct  self)
{
  usize D.206;

  _1 = self.0;
  _2 = self.1;
  D.206 = _1 + _2;
  return D.206;
}


usize f ()
{
  usize D.208;
  struct  D.209;

  D.209.0 = 123;
  D.209.1 = 42;
  D.208 = S_sum (D.209);
  return D.208;
}

@bjorn3
Copy link

bjorn3 commented Jul 5, 2021

Unlike LLVM, GCC doesn't assume -fno-semantic-interposition, so it is valid to replace TestCrate::S::sum with a different function if it is exported. Specifying -fno-semantic-interposition doesn't seem to help though, though it should probably be the default at least for non-#[no_mangle] symbols. By the way when checking "compile to binary" I see several __morestack calls that complicate the disassembly a lot.

@philberty
Copy link
Member

Also it looks like there might be another bug since its referencing usize here not u64

@philberty
Copy link
Member

Found the bug with the gimple reference to 'usize' it was a bad call to named_type to give the integer type a relevant name in gimple land. The underlying integer type was correct but the name was bad.

bors bot added a commit that referenced this issue Jul 6, 2021
548: Fix bad naming of primitive types such as u64 which ended up as usize r=philberty a=philberty

In #547 it was found that u64's ended up as usize in gimple which confuses
debugging sessions. This take the original implementation of named type
from gccgo showing it was the TYPE_NAME tree getting confused.

Addresses #547

Co-authored-by: Philip Herron <philip.herron@embecosm.com>
@philberty
Copy link
Member

Found the issue is here: https://github.com/Rust-GCC/gccrs/blob/master/gcc/rust/rust-gcc.cc#L3373-L3374

We need to explicitly set function_is_inlinable flags other wise the rust-gcc.cc wrapper ensures that this function is un-inlinable. I think it makes sense being able to turn this on and off, so lets make the flags default allow for inlining

philberty added a commit that referenced this issue Sep 14, 2021
We are reusing a forked version of the GCC wrapper from the go front-end,
if we do not specify that a function is inlineable, this marks the function
as DECL_UNINLINABLE which means we miss alot of optimization opertunities.

Fixes #547
@philberty philberty added this to the Data Structures 3 - Traits milestone Sep 14, 2021
@bors bors bot closed this as completed in #673 Sep 14, 2021
@bors bors bot closed this as completed in 4493f1c Sep 14, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants