[PATCH] D117795: [AArch64] Add some missing strict FP vector lowering

[John Brawn via Phabricator via cfe-commits]

john.brawn planned changes to this revision.
john.brawn added inline comments.


================
Comment at: llvm/lib/Target/AArch64/AArch64ISelLowering.cpp:1484
+  // of the vector comparison instructions.
+  setOperationAction(ISD::STRICT_FSETCCS, VT, Expand);
+  // FIXME: We could potentially make use of the vector comparison instructions
----------------
dmgreen wrote:
> john.brawn wrote:
> > dmgreen wrote:
> > > Can you split this into a separate patch? I know I sound like a broken record, but it doesn't seem to be related to the converts below.
> > > 
> > > Also pre-committing as much of the test that works as possible would cut it down from this patch quite a bit.
> > Instead of a separate patch just for these two, it would probably make more sense to move them into D114946 with the rest of the setOperationAction lines.
> > 
> > On the test, without the changes in this patch it hits an assertion failure so as a separate commit before this it wouldn't be able to test anything.
> D114946 is already pretty big :)
> 
> What about the add_v4f32 test (for example) requires the changes in this patch? That's what I meant by "precommit as much as possible".
> 
> My other question about this was going to be - why can't we use the vector instructions for STRICT_FSETCCS? The FCMGE and FCMGT look like they would set exception flags to me, but I may be misunderstanding some part of it.
> D114946 is already pretty big :)
A bit, though only in terms of number of lines. Conceptually it fairly simple, as it's almost all "strict_xyz can be handled the same as (non-strict) xyz".

> What about the add_v4f32 test (for example) requires the changes in this patch? That's what I meant by "precommit as much as possible".
I _could_ try and disentangle the parts that compile without this patch, but it seems like more work than it's worth.

> My other question about this was going to be - why can't we use the vector instructions for STRICT_FSETCCS? The FCMGE and FCMGT look like they would set exception flags to me, but I may be misunderstanding some part of it.

Hmm, it looks like the FCMXY instructions are inconsistent in their handling of NaNs. FCMEQ performs a quiet comparison (only signalling NaNs raise an exception) like FCMP and STRICT_FSETCC, FCMGT etc. performs a signalling comparison (all NaNs raise an exception) like FCMPE and STRICT_FSETCCS. I'll adjust the comments (and also move this to D114946).


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D117795/new/

https://reviews.llvm.org/D117795