Re: [Tinycc-devel] [PATCH] When handling '.' operator, cope with VT_LLOCAL

[Michael Matz]

Hi,

On Sat, 21 Feb 2015, Edmund Grimley Evans wrote:

> > Why are the
> > others trivially correct and don't need such handling?
>
> I expect there are many bits of code that are wrong but which happen
> never to receive the values that they can't handle.

Well, I'd hope we can do a bit better than this.  For instance by 
introducing a new function that does what lvalue_type does, _plus_ the 
VT_LLOCAL handling, plus changing vtop->r (or any given SValue*).

> How many parts of tccgen.c can correctly handle a VT_REF, do you
> think? :-)

Heh.  Although VT_REF is probably a bit easier, because it doesn't get 
reintroduced once it went away from a SValue node.  Really, I'm all in 
favor of dumping VT_REF, I just want us to not leave it in for one target 
when it's not really necessary but can be replaced with a fixed VT_LLOCAL.

> The advantage of making gen_op always convert to an rvalue is that other 
> code would have to worry about fewer cases.
>
> The only disadvantage that I can think of is that certain expressions 
> which would cause GCC to warn that "value computed is not used" would 
> not get optimised.

I think the conversion to rvalue might not actually have to emit code, in 
which case unused stuff would be as optimized as before (as far as tcc is 
optimizing :)).

> So, if you're suggesting that gen_op should be modified to always 
> convert to an rvalue, then I think I agree. Shall we do that instead?

I'd try it at least and see if it works.

> Since I probably still have a test case to hand, should I propose an 
> alternative patch that fixes my problem by modifying gen_op?

Yeah, I think so.  As testcase this one works as well (compile only and 
inspect asm manually):

struct S {int a; int b; int x[7];};
int f (int a, int b, int c, int d, struct S s)
{
  return s.a;
}

Compiling with the x86_64-w64-mingw32-tcc cross compiler should result in 
such code:

0000000000000000 <f>:
   0:   55                      push   %rbp
   1:   48 89 e5                mov    %rsp,%rbp
   4:   48 81 ec 00 00 00 00    sub    $0x0,%rsp
   b:   48 89 4d 10             mov    %rcx,0x10(%rbp)
   f:   48 89 55 18             mov    %rdx,0x18(%rbp)
  13:   4c 89 45 20             mov    %r8,0x20(%rbp)
  17:   4c 89 4d 28             mov    %r9,0x28(%rbp)
  1b:   48 8b 45 30             mov    0x30(%rbp),%rax   # 1
  1f:   8b 00                   mov    (%rax),%eax       # 2
  21:   e9 00 00 00 00          jmpq   26 <f+0x26>
  26:   c9                      leaveq
  27:   c3                      retq
  28:   01 04 02                add    %eax,(%rdx,%rax,1)
  2b:   05 04 03 01 50          add    $0x50010304,%eax

There must be a load from 0x30(%rbp) (#1), and a further load from that 
loaded address (#2) into the result register %eax.

With the change in x86_64-gen.c I asked grischka to test 
(VT_LOCAL|VT_REF->VT_LLOCAL), but without your fix to the '.' operator 
this will generate wrong code (load #2 will be missing).

But it will be nice if you can create a short runtime testcase, even if it 
fails (without fixes) only on aarch64.


Ciao,
Michael.