xiezhiheng <xiezhih...@huawei.com> writes: >> -----Original Message----- >> From: Richard Sandiford [mailto:richard.sandif...@arm.com] >> Sent: Friday, July 31, 2020 5:03 PM >> To: xiezhiheng <xiezhih...@huawei.com> >> Cc: Richard Biener <richard.guent...@gmail.com>; gcc-patches@gcc.gnu.org >> Subject: Re: [PATCH PR94442] [AArch64] Redundant ldp/stp instructions >> emitted at -O3 >> >> xiezhiheng <xiezhih...@huawei.com> writes: >> >> -----Original Message----- >> >> From: Richard Sandiford [mailto:richard.sandif...@arm.com] >> >> Sent: Friday, July 17, 2020 5:04 PM >> >> To: xiezhiheng <xiezhih...@huawei.com> >> >> Cc: Richard Biener <richard.guent...@gmail.com>; >> gcc-patches@gcc.gnu.org >> >> Subject: Re: [PATCH PR94442] [AArch64] Redundant ldp/stp instructions >> >> emitted at -O3 >> >> >> > >> > Cut... >> > >> >> >> >> Thanks, pushed to master. >> >> >> >> Richard >> > >> > And I have finished the second part. >> > >> > In function aarch64_general_add_builtin, I add an argument ATTRS to >> > pass attributes for each built-in function. >> > >> > And some new functions are added: >> > aarch64_call_properties: return flags for each built-in function based >> > on command-line options. When the built-in function handles >> > floating-points, add FLAG_FP flag. >> > >> > aarch64_modifies_global_state_p: True if the function would modify >> > global states. >> > >> > aarch64_reads_global_state_p: True if the function would read >> > global states. >> > >> > aarch64_could_trap_p: True if the function would raise a signal. >> > >> > aarch64_add_attribute: Add attributes in ATTRS. >> > >> > aarch64_get_attributes: return attributes for each built-in functons >> > based on flags and command-line options. >> > >> > In function aarch64_init_simd_builtins, attributes are get by flags >> > and pass them to function aarch64_general_add_builtin. >> > >> > >> > Bootstrap is tested OK on aarch64 Linux platform, but regression >> > FAIL one test case ---- pr93423.f90. >> > However, I found that this test case would fail randomly in trunk. >> > https://gcc.gnu.org/bugzilla/show_bug.cgi?id=93423 >> > https://gcc.gnu.org/bugzilla/show_bug.cgi?id=96041 >> > Some PRs have tracked it. After my patch, this test case would >> > always fail. I guess the syntax errors in fortran crash some structures >> > result in illegal memory access but I can't find what exactly it is. >> > But I think my patch should have no influence on it. >> >> Yeah, I agree. And FWIW, I didn't see this in my testing. >> >> I've pushed the patch with one trivial change: to remove the “and” >> before “CODE” in: >> >> > /* Wrapper around add_builtin_function. NAME is the name of the >> built-in >> > function, TYPE is the function type, and CODE is the function subcode >> > - (relative to AARCH64_BUILTIN_GENERAL). */ >> > + (relative to AARCH64_BUILTIN_GENERAL), and ATTRS is the function >> > + attributes. */ >> >> BTW, one thing to be careful of in future is that not all FP intrinsics >> raise FP exceptions. So while: >> >> > + switch (d->mode) >> > + { >> > + /* Floating-point. */ >> > + case E_BFmode: >> > + case E_V4BFmode: >> > + case E_V8BFmode: >> > + case E_HFmode: >> > + case E_V4HFmode: >> > + case E_V8HFmode: >> > + case E_SFmode: >> > + case E_V2SFmode: >> > + case E_V4SFmode: >> > + case E_DFmode: >> > + case E_V1DFmode: >> > + case E_V2DFmode: >> > + flags |= FLAG_FP; >> > + break; >> > + >> > + default: >> > + break; >> > + } >> >> is a good, conservatively-correct default, we might need an additional >> flag to suppress it for certain intrinsics. >> > > I agree. > >> I've just realised that the code above could have used FLOAT_MODE_P, >> but I didn't think of that before pushing the patch :-) >> > > Sorry, I should have used it. And I prepare a patch to use FLOAT_MODE_P > macro and add a flag FLAG_SUPPRESS_FP_EXCEPTIONS to suppress > FLAG_RAISE_FP_EXCEPTIONS for certain intrinsics in future.
The same thing is true for reading FPCR as well, so I think the flag should suppress the FLOAT_MODE_P check, instead of fixing up the flags afterwards. I'm struggling to think of a good name though. How about adding FLAG_AUTO_FP and making the FLOAT_MODE_P check dependent on FLAG_AUTO_FP being set? We could leave FLAG_AUTO_FP out of FLAG_ALL, since FLAG_ALL already includes FLAG_FP. Including it in FLAG_ALL wouldn't do no any harm though. Thanks, Richard
