Use the latest base register while accessing the stack

[kunalspathak]

When we try to access a field of a struct parameter, we calculate the offset of field using wrong base register. The struct that is getting accessed has 257 fields and we use sp as a base register to calculate the offset. However at 152nd field we reach the encoding limit of 0xFF8 and hence decide to use fp as a new base register to calculate the offset from. But we don't take the new base register into account when calculating the final indirect of the field.

Here is the snippet of code that loads each field .

 movw r10,0xfe8addr10,sp vldr d10,[r10] ; field access movw r10,0xff0addr10,sp vldr d10,[r10] ; field access movw r10,0xff8addr10,sp vldr d10,[r10] ; field access movw r10,0x4c0addr10,sp ; <-- this should be "add r10, r11" vldr d10,[r10] movw r10,0x4c8addr10,sp vldr d10,[r10]

The fix is to use the latest base register we find out (through lvaFrameAddress) inside emitIns_R_S() when called with INS_movw and propagate it via emitIns_genStackOffset(). In order to come up with a small repro case, I wanted to see how this situation arises with stress flags, but couldn't find exact reason. One thing that I noticed was in repro case, the compLclFrameSize is lot bigger than the non-repro case, but again was not sure enough.

I ran various jitstress pipelines and looks like the mcc_i* tests are passing. Here is the test log.

Fixes: #35501 and #36199

[kunalspathak]

@dotnet/jit-contrib

[CarolEidt]

The changes seem reasonable, but I would probably have used a return value for the base register (since these methods are currently void) rather than adding a pointer as an "out" parameter.

[kunalspathak]

The changes seem reasonable, but I would probably have used a return value for the base register (since these methods are currently void) rather than adding a pointer as an "out" parameter.

Thanks @CarolEidt . My first thought was to return a value but since it was one of the emitIns_* methods which has return signature as void, I do not wanted to break the consistency. If it is ok with you, I will change it to return instead of out parameter.

[CarolEidt]

If it is ok with you, I will change it to return instead of out parameter.

To me, the inconsistency actually makes it clearer that these methods are "special", though I supposed there's also the danger that the caller will ignore the return value, since C++ makes it easy to do so. Do others have different opinions? @dotnet/jit-contrib

[echesakov]

Do others have different opinions? @dotnet/jit-contrib

I would just go with return type = void and returning the base register via out parameter (that should be named pBaseReg)

[CarolEidt]

LGTM

[CarolEidt]

added standard function header

Thanks!

[kunalspathak]

When we try to access a field of a struct parameter, we calculate the offset of field using wrong base register. The struct that is getting accessed has 257 fields and we use sp as a base register to calculate the offset. However at 152nd field we reach the encoding limit of 0xFF8 and hence decide to use fp as a new base register to calculate the offset from. But we don't take the new base register into account when calculating the final indirect of the field.

Here is the snippet of code that loads each field .

 movw r10,0xfe8addr10,sp vldr d10,[r10] ; field access movw r10,0xff0addr10,sp vldr d10,[r10] ; field access movw r10,0xff8addr10,sp vldr d10,[r10] ; field access movw r10,0x4c0addr10,sp ; <-- this should be "add r10, r11" vldr d10,[r10] movw r10,0x4c8addr10,sp vldr d10,[r10]

The fix is to use the latest base register we find out (through lvaFrameAddress) inside emitIns_R_S() when called with INS_movw and propagate it via emitIns_genStackOffset(). In order to come up with a small repro case, I wanted to see how this situation arises with stress flags, but couldn't find exact reason. One thing that I noticed was in repro case, the compLclFrameSize is lot bigger than the non-repro case, but again was not sure enough.

I ran various jitstress pipelines and looks like the mcc_i* tests are passing. Here is the test log.

Fixes: #35501 and #36199

[kunalspathak]

@dotnet/jit-contrib

[CarolEidt]

The changes seem reasonable, but I would probably have used a return value for the base register (since these methods are currently void) rather than adding a pointer as an "out" parameter.

[kunalspathak]

The changes seem reasonable, but I would probably have used a return value for the base register (since these methods are currently void) rather than adding a pointer as an "out" parameter.

Thanks @CarolEidt . My first thought was to return a value but since it was one of the emitIns_* methods which has return signature as void, I do not wanted to break the consistency. If it is ok with you, I will change it to return instead of out parameter.

[CarolEidt]

If it is ok with you, I will change it to return instead of out parameter.

To me, the inconsistency actually makes it clearer that these methods are "special", though I supposed there's also the danger that the caller will ignore the return value, since C++ makes it easy to do so. Do others have different opinions? @dotnet/jit-contrib

[echesakov]

Do others have different opinions? @dotnet/jit-contrib

I would just go with return type = void and returning the base register via out parameter (that should be named pBaseReg)

[CarolEidt]

LGTM

[CarolEidt]

added standard function header

Thanks!

[kunalspathak]

When we try to access a field of a struct parameter, we calculate the offset of field using wrong base register. The struct that is getting accessed has 257 fields and we use sp as a base register to calculate the offset. However at 152nd field we reach the encoding limit of 0xFF8 and hence decide to use fp as a new base register to calculate the offset from. But we don't take the new base register into account when calculating the final indirect of the field.

Here is the snippet of code that loads each field .

 movw r10,0xfe8addr10,sp vldr d10,[r10] ; field access movw r10,0xff0addr10,sp vldr d10,[r10] ; field access movw r10,0xff8addr10,sp vldr d10,[r10] ; field access movw r10,0x4c0addr10,sp ; <-- this should be "add r10, r11" vldr d10,[r10] movw r10,0x4c8addr10,sp vldr d10,[r10]

The fix is to use the latest base register we find out (through lvaFrameAddress) inside emitIns_R_S() when called with INS_movw and propagate it via emitIns_genStackOffset(). In order to come up with a small repro case, I wanted to see how this situation arises with stress flags, but couldn't find exact reason. One thing that I noticed was in repro case, the compLclFrameSize is lot bigger than the non-repro case, but again was not sure enough.

I ran various jitstress pipelines and looks like the mcc_i* tests are passing. Here is the test log.

Fixes: #35501 and #36199

[kunalspathak]

@dotnet/jit-contrib

[CarolEidt]

The changes seem reasonable, but I would probably have used a return value for the base register (since these methods are currently void) rather than adding a pointer as an "out" parameter.

[kunalspathak]

The changes seem reasonable, but I would probably have used a return value for the base register (since these methods are currently void) rather than adding a pointer as an "out" parameter.

Thanks @CarolEidt . My first thought was to return a value but since it was one of the emitIns_* methods which has return signature as void, I do not wanted to break the consistency. If it is ok with you, I will change it to return instead of out parameter.

[CarolEidt]

If it is ok with you, I will change it to return instead of out parameter.

To me, the inconsistency actually makes it clearer that these methods are "special", though I supposed there's also the danger that the caller will ignore the return value, since C++ makes it easy to do so. Do others have different opinions? @dotnet/jit-contrib

[echesakov]

Do others have different opinions? @dotnet/jit-contrib

I would just go with return type = void and returning the base register via out parameter (that should be named pBaseReg)

[CarolEidt]

LGTM

[CarolEidt]

added standard function header

Thanks!

[kunalspathak]

When we try to access a field of a struct parameter, we calculate the offset of field using wrong base register. The struct that is getting accessed has 257 fields and we use sp as a base register to calculate the offset. However at 152nd field we reach the encoding limit of 0xFF8 and hence decide to use fp as a new base register to calculate the offset from. But we don't take the new base register into account when calculating the final indirect of the field.

Here is the snippet of code that loads each field .

 movw r10,0xfe8addr10,sp vldr d10,[r10] ; field access movw r10,0xff0addr10,sp vldr d10,[r10] ; field access movw r10,0xff8addr10,sp vldr d10,[r10] ; field access movw r10,0x4c0addr10,sp ; <-- this should be "add r10, r11" vldr d10,[r10] movw r10,0x4c8addr10,sp vldr d10,[r10]

The fix is to use the latest base register we find out (through lvaFrameAddress) inside emitIns_R_S() when called with INS_movw and propagate it via emitIns_genStackOffset(). In order to come up with a small repro case, I wanted to see how this situation arises with stress flags, but couldn't find exact reason. One thing that I noticed was in repro case, the compLclFrameSize is lot bigger than the non-repro case, but again was not sure enough.

I ran various jitstress pipelines and looks like the mcc_i* tests are passing. Here is the test log.

Fixes: #35501 and #36199

[kunalspathak]

@dotnet/jit-contrib

[CarolEidt]

The changes seem reasonable, but I would probably have used a return value for the base register (since these methods are currently void) rather than adding a pointer as an "out" parameter.

[kunalspathak]

The changes seem reasonable, but I would probably have used a return value for the base register (since these methods are currently void) rather than adding a pointer as an "out" parameter.

Thanks @CarolEidt . My first thought was to return a value but since it was one of the emitIns_* methods which has return signature as void, I do not wanted to break the consistency. If it is ok with you, I will change it to return instead of out parameter.

[CarolEidt]

If it is ok with you, I will change it to return instead of out parameter.

To me, the inconsistency actually makes it clearer that these methods are "special", though I supposed there's also the danger that the caller will ignore the return value, since C++ makes it easy to do so. Do others have different opinions? @dotnet/jit-contrib

[echesakov]

Do others have different opinions? @dotnet/jit-contrib

I would just go with return type = void and returning the base register via out parameter (that should be named pBaseReg)

[CarolEidt]

LGTM

[CarolEidt]

added standard function header

Thanks!

[kunalspathak]

When we try to access a field of a struct parameter, we calculate the offset of field using wrong base register. The struct that is getting accessed has 257 fields and we use sp as a base register to calculate the offset. However at 152nd field we reach the encoding limit of 0xFF8 and hence decide to use fp as a new base register to calculate the offset from. But we don't take the new base register into account when calculating the final indirect of the field.

Here is the snippet of code that loads each field .

 movw r10,0xfe8addr10,sp vldr d10,[r10] ; field access movw r10,0xff0addr10,sp vldr d10,[r10] ; field access movw r10,0xff8addr10,sp vldr d10,[r10] ; field access movw r10,0x4c0addr10,sp ; <-- this should be "add r10, r11" vldr d10,[r10] movw r10,0x4c8addr10,sp vldr d10,[r10]

The fix is to use the latest base register we find out (through lvaFrameAddress) inside emitIns_R_S() when called with INS_movw and propagate it via emitIns_genStackOffset(). In order to come up with a small repro case, I wanted to see how this situation arises with stress flags, but couldn't find exact reason. One thing that I noticed was in repro case, the compLclFrameSize is lot bigger than the non-repro case, but again was not sure enough.

I ran various jitstress pipelines and looks like the mcc_i* tests are passing. Here is the test log.

Fixes: #35501 and #36199

[kunalspathak]

@dotnet/jit-contrib

[CarolEidt]

The changes seem reasonable, but I would probably have used a return value for the base register (since these methods are currently void) rather than adding a pointer as an "out" parameter.

[kunalspathak]

The changes seem reasonable, but I would probably have used a return value for the base register (since these methods are currently void) rather than adding a pointer as an "out" parameter.

Thanks @CarolEidt . My first thought was to return a value but since it was one of the emitIns_* methods which has return signature as void, I do not wanted to break the consistency. If it is ok with you, I will change it to return instead of out parameter.

[CarolEidt]

If it is ok with you, I will change it to return instead of out parameter.

To me, the inconsistency actually makes it clearer that these methods are "special", though I supposed there's also the danger that the caller will ignore the return value, since C++ makes it easy to do so. Do others have different opinions? @dotnet/jit-contrib

[echesakov]

Do others have different opinions? @dotnet/jit-contrib

I would just go with return type = void and returning the base register via out parameter (that should be named pBaseReg)

[CarolEidt]

LGTM

[CarolEidt]

added standard function header

Thanks!

[kunalspathak]

When we try to access a field of a struct parameter, we calculate the offset of field using wrong base register. The struct that is getting accessed has 257 fields and we use sp as a base register to calculate the offset. However at 152nd field we reach the encoding limit of 0xFF8 and hence decide to use fp as a new base register to calculate the offset from. But we don't take the new base register into account when calculating the final indirect of the field.

Here is the snippet of code that loads each field .

 movw r10,0xfe8addr10,sp vldr d10,[r10] ; field access movw r10,0xff0addr10,sp vldr d10,[r10] ; field access movw r10,0xff8addr10,sp vldr d10,[r10] ; field access movw r10,0x4c0addr10,sp ; <-- this should be "add r10, r11" vldr d10,[r10] movw r10,0x4c8addr10,sp vldr d10,[r10]

The fix is to use the latest base register we find out (through lvaFrameAddress) inside emitIns_R_S() when called with INS_movw and propagate it via emitIns_genStackOffset(). In order to come up with a small repro case, I wanted to see how this situation arises with stress flags, but couldn't find exact reason. One thing that I noticed was in repro case, the compLclFrameSize is lot bigger than the non-repro case, but again was not sure enough.

I ran various jitstress pipelines and looks like the mcc_i* tests are passing. Here is the test log.

Fixes: #35501 and #36199

[kunalspathak]

@dotnet/jit-contrib

[CarolEidt]

The changes seem reasonable, but I would probably have used a return value for the base register (since these methods are currently void) rather than adding a pointer as an "out" parameter.

[kunalspathak]

The changes seem reasonable, but I would probably have used a return value for the base register (since these methods are currently void) rather than adding a pointer as an "out" parameter.

Thanks @CarolEidt . My first thought was to return a value but since it was one of the emitIns_* methods which has return signature as void, I do not wanted to break the consistency. If it is ok with you, I will change it to return instead of out parameter.

[CarolEidt]

If it is ok with you, I will change it to return instead of out parameter.

To me, the inconsistency actually makes it clearer that these methods are "special", though I supposed there's also the danger that the caller will ignore the return value, since C++ makes it easy to do so. Do others have different opinions? @dotnet/jit-contrib

[echesakov]

Do others have different opinions? @dotnet/jit-contrib

I would just go with return type = void and returning the base register via out parameter (that should be named pBaseReg)

[CarolEidt]

LGTM

[CarolEidt]

added standard function header

Thanks!

[kunalspathak]

When we try to access a field of a struct parameter, we calculate the offset of field using wrong base register. The struct that is getting accessed has 257 fields and we use sp as a base register to calculate the offset. However at 152nd field we reach the encoding limit of 0xFF8 and hence decide to use fp as a new base register to calculate the offset from. But we don't take the new base register into account when calculating the final indirect of the field.

Here is the snippet of code that loads each field .

 movw r10,0xfe8addr10,sp vldr d10,[r10] ; field access movw r10,0xff0addr10,sp vldr d10,[r10] ; field access movw r10,0xff8addr10,sp vldr d10,[r10] ; field access movw r10,0x4c0addr10,sp ; <-- this should be "add r10, r11" vldr d10,[r10] movw r10,0x4c8addr10,sp vldr d10,[r10]

The fix is to use the latest base register we find out (through lvaFrameAddress) inside emitIns_R_S() when called with INS_movw and propagate it via emitIns_genStackOffset(). In order to come up with a small repro case, I wanted to see how this situation arises with stress flags, but couldn't find exact reason. One thing that I noticed was in repro case, the compLclFrameSize is lot bigger than the non-repro case, but again was not sure enough.

I ran various jitstress pipelines and looks like the mcc_i* tests are passing. Here is the test log.

Fixes: #35501 and #36199

[kunalspathak]

@dotnet/jit-contrib

[CarolEidt]

The changes seem reasonable, but I would probably have used a return value for the base register (since these methods are currently void) rather than adding a pointer as an "out" parameter.

[kunalspathak]

The changes seem reasonable, but I would probably have used a return value for the base register (since these methods are currently void) rather than adding a pointer as an "out" parameter.

Thanks @CarolEidt . My first thought was to return a value but since it was one of the emitIns_* methods which has return signature as void, I do not wanted to break the consistency. If it is ok with you, I will change it to return instead of out parameter.

[CarolEidt]

If it is ok with you, I will change it to return instead of out parameter.

To me, the inconsistency actually makes it clearer that these methods are "special", though I supposed there's also the danger that the caller will ignore the return value, since C++ makes it easy to do so. Do others have different opinions? @dotnet/jit-contrib

[echesakov]

Do others have different opinions? @dotnet/jit-contrib

I would just go with return type = void and returning the base register via out parameter (that should be named pBaseReg)

[CarolEidt]

LGTM

[CarolEidt]

added standard function header

Thanks!

[kunalspathak]

When we try to access a field of a struct parameter, we calculate the offset of field using wrong base register. The struct that is getting accessed has 257 fields and we use sp as a base register to calculate the offset. However at 152nd field we reach the encoding limit of 0xFF8 and hence decide to use fp as a new base register to calculate the offset from. But we don't take the new base register into account when calculating the final indirect of the field.

Here is the snippet of code that loads each field .

 movw r10,0xfe8addr10,sp vldr d10,[r10] ; field access movw r10,0xff0addr10,sp vldr d10,[r10] ; field access movw r10,0xff8addr10,sp vldr d10,[r10] ; field access movw r10,0x4c0addr10,sp ; <-- this should be "add r10, r11" vldr d10,[r10] movw r10,0x4c8addr10,sp vldr d10,[r10]

The fix is to use the latest base register we find out (through lvaFrameAddress) inside emitIns_R_S() when called with INS_movw and propagate it via emitIns_genStackOffset(). In order to come up with a small repro case, I wanted to see how this situation arises with stress flags, but couldn't find exact reason. One thing that I noticed was in repro case, the compLclFrameSize is lot bigger than the non-repro case, but again was not sure enough.

I ran various jitstress pipelines and looks like the mcc_i* tests are passing. Here is the test log.

Fixes: #35501 and #36199

[kunalspathak]

@dotnet/jit-contrib

[CarolEidt]

The changes seem reasonable, but I would probably have used a return value for the base register (since these methods are currently void) rather than adding a pointer as an "out" parameter.

[kunalspathak]

The changes seem reasonable, but I would probably have used a return value for the base register (since these methods are currently void) rather than adding a pointer as an "out" parameter.

Thanks @CarolEidt . My first thought was to return a value but since it was one of the emitIns_* methods which has return signature as void, I do not wanted to break the consistency. If it is ok with you, I will change it to return instead of out parameter.

[CarolEidt]

If it is ok with you, I will change it to return instead of out parameter.

To me, the inconsistency actually makes it clearer that these methods are "special", though I supposed there's also the danger that the caller will ignore the return value, since C++ makes it easy to do so. Do others have different opinions? @dotnet/jit-contrib

[echesakov]

Do others have different opinions? @dotnet/jit-contrib

I would just go with return type = void and returning the base register via out parameter (that should be named pBaseReg)

[CarolEidt]

LGTM

[CarolEidt]

added standard function header

Thanks!