arc: fix register constraints in extvsi patterns#269
Conversation
| @@ -0,0 +1,60 @@ | |||
| /* { dg-do run } */ | |||
There was a problem hiding this comment.
I think a run test is a bit overkill here.
I would keep it simple and just use a test that checks the generated asm.
There was a problem hiding this comment.
I did add checks for the produced assembly code but i think there is need to keep the execution test in place.
Converting this test to a compile only check gives visibility in the final code generation - but regarding correctness is not enough. The issues addressed here have produced incorrect executions (with several people involved in fixing) that was spotted by clients - but looking inside the produced code was not obvious the real reason. Unlike LLVM lit - which can isolate and dump intermediate representation dejagnu can only scan the end of pipeline assembly file. For example checking for "sub.*,65536" only proves that the instruction exists at the end of compilation. It cannot prove that the pattern split successfully took place during the split1 phase. A later optimization or peephole pass could be resynthesizing such instructions giving us a false positive while masking a broken splitter. Also checking the produced assembly does not validate data flow or correct register allocation. The compiler can easily emit the expected bxor and sub sequences but read from an uninitialized register.
The patch for the upstream is here:
#270
| (define_insn_and_split "*extvsi_1_0" | ||
| [(set (match_operand:SI 0 "register_operand" "=r") | ||
| (sign_extract:SI (match_operand:SI 1 "register_operand" "0") | ||
| (sign_extract:SI (match_operand:SI 1 "register_operand" "r") |
There was a problem hiding this comment.
Can you focus on addressing the issue upstream first?
Part of trying to upstream is to also question the decisions made by Claudiu there for the extvsi_n_0 pattern.
There was a problem hiding this comment.
We will switch to GCC 16 for the next release. So it is preferable to focus on the upstream version of this.
The multi-bit extvsi_n_0 and single-bit extvsi_1_0 signed bitfield extraction patterns were utilizing a too restrictive matching constraint "0" for operand 1 while splitting early (&& 1) before ra/reload. Because early splits occur prior to the reload and register allocation passes when operands are still separate pseudos, this mismatch violates rtl dataflow constraints as it can confuse data flow. Remark that in the upstream gcc this patch wont apply as there are other split conditions put in place (although again too restrictive).
Alexehv77
force-pushed
the
fix_extvsi_patterns
branch
from
aea168e to
b18dd9b
Compare
|
i have created a new pr with the changes for upstream. |
2 participants
The multi-bit extvsi_n_0 and single-bit extvsi_1_0 signed bitfield extraction patterns were utilizing a too restrictive matching constraint "0" for operand 1 while splitting early (&& 1) before ra/reload.
Because early splits occur prior to the reload and register allocation passes when operands are still separate pseudos, this mismatch violates rtl dataflow constraints as it can confuse data flow.
Remark that in the upstream gcc this patch wont apply as there are other split conditions put in place (although again too restrictive).