This is the mail archive of the gcc-patches@gcc.gnu.org mailing list for the GCC project.

Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]
Other format: [Raw text]

Re: [PATCH PR82096] Fix ICE in int_mode_for_mode, at stor-layout.c:403 with arm-linux-gnueabi

Hi Jeff

On 12/01/18 23:00, Jeff Law wrote:

On 01/12/2018 01:45 AM, Christophe Lyon wrote:

Hi,

On 11 January 2018 at 11:58, Sudakshina Das <sudi.das@arm.com> wrote:

Hi Jeff


On 10/01/18 21:08, Jeff Law wrote:


On 01/10/2018 09:25 AM, Sudakshina Das wrote:


Hi Jeff

On 10/01/18 10:44, Sudakshina Das wrote:


Hi Jeff

On 09/01/18 23:43, Jeff Law wrote:


On 01/05/2018 12:25 PM, Sudakshina Das wrote:


Hi Jeff

On 05/01/18 18:44, Jeff Law wrote:


On 01/04/2018 08:35 AM, Sudakshina Das wrote:


Hi

The bug reported a particular test di-longlong64-sync-1.c failing
when
run on arm-linux-gnueabi with options -mthumb -march=armv5t
-O[g,1,2,3]
and -mthumb -march=armv6 -O[g,1,2,3].

According to what I could see, the crash was caused because of the
explicit VOIDmode argument that was sent to emit_store_flag_force
().
Since the comparing argument was a long long, it was being forced
into a
VOID type register before the comparison (in prepare_cmp_insn()) is
done.



As pointed out by Kyrill, there is a comment on emit_store_flag()
which
says "MODE is the mode to use for OP0 and OP1 should they be
CONST_INTs.
     If it is VOIDmode, they cannot both be CONST_INT". This
condition is
not true in this case and thus I think it is suitable to change the
argument.

Testing done: Checked for regressions on bootstrapped
arm-none-linux-gnueabi and arm-none-linux-gnueabihf and added new
test
cases.

Sudi

ChangeLog entries:

*** gcc/ChangeLog ***

2017-01-04  Sudakshina Das  <sudi.das@arm.com>

        PR target/82096
        * optabs.c (expand_atomic_compare_and_swap): Change argument
        to emit_store_flag_force.

*** gcc/testsuite/ChangeLog ***

2017-01-04  Sudakshina Das  <sudi.das@arm.com>

        PR target/82096
        * gcc.c-torture/compile/pr82096-1.c: New test.
        * gcc.c-torture/compile/pr82096-2.c: Likwise.


In the case where both (op0/op1) to
emit_store_flag/emit_store_flag_force are constants, don't we know
the
result of the comparison and shouldn't we have optimized the store
flag
to something simpler?

I feel like I must be missing something here.



emit_store_flag_force () is comparing a register to op0.


?
/* Emit a store-flags instruction for comparison CODE on OP0 and OP1
      and storing in TARGET.  Normally return TARGET.
      Return 0 if that cannot be done.

      MODE is the mode to use for OP0 and OP1 should they be
CONST_INTs.  If
      it is VOIDmode, they cannot both be CONST_INT.


So we're comparing op0 and op1 AFAICT.  One, but not both can be a
CONST_INT.  If both are a CONST_INT, then you need to address the
problem in the caller (by optimizing away the condition).  If you've
got
a REG and a CONST_INT, then the mode should be taken from the REG
operand.







The 2 constant arguments are to the expand_atomic_compare_and_swap ()
function. emit_store_flag_force () is used in case when this
function is
called by the bool variant of the built-in function where the bool
return value is computed by comparing the result register with the
expected op0.


So if only one of the two objects is a CONST_INT, then the mode should
come from the other object.  I think that's the fundamental problem
here
and that you're just papering over it by changing the caller.


I think my earlier explanation was a bit misleading and I may have
rushed into quoting the comment about both operands being const for
emit_store_flag_force(). The problem is with the function and I do
agree with your suggestion of changing the function to add the code
below to be a better approach than the changing the caller. I will
change the patch and test it.



This is the updated patch according to your suggestions.

Testing: Checked for regressions on arm-none-linux-gnueabihf and added
new test case.

Thanks
Sudi

ChangeLog entries:

*** gcc/ChangeLog ***

2017-01-10  Sudakshina Das  <sudi.das@arm.com>

      PR target/82096
      * expmed.c (emit_store_flag_force): Swap if const op0
      and change VOIDmode to mode of op0.

*** gcc/testsuite/ChangeLog ***

2017-01-10  Sudakshina Das  <sudi.das@arm.com>

      PR target/82096
      * gcc.c-torture/compile/pr82096.c: New test.


OK.



Thanks. Committed as r256526.
Sudi



Could you add a guard like in other tests to skip it if the user added
-mfloat-abi=XXX when running the tests?

For instance, I have a configuration where I add
-mthumb/-march=armv8-a/-mfpu=crypto-neon-fp-armv8/-mfloat-abi=hard
and the new test fails because:
xgcc: error: -mfloat-abi=soft and -mfloat-abi=hard may not be used together

It's starting to feel like the test should move into gcc.target/arm :-)
  I nearly suggested that already.  Consider moving it into
gcc.target/arm pre-approved along with adding the -O<whatever you need>
to the options and whatever is needed to skip the test at the
appropriate time.
My initial thought was also to put the test in gcc.target/arm. But I wanted to put it in a torture suite as this was failing at different optimization levels. Creating several tests for different optimization levels or a new torture suite just for this test did not look like the better options in the end and hence I put it here.
In order to fix this test, I have proposed the following patch https://gcc.gnu.org/ml/gcc-patches/2018-01/msg01420.html last week. I have added a further directive this morning just in case. 

Thanks
Sudi




jeff
Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]