[PATCH] D59062: [GlobalISel][AArch64] Always fall back on aarch64.neon.addp.*

[Jessica Paquette via Phabricator via llvm-commits]

paquette marked an inline comment as done.
paquette added inline comments.


================
Comment at: lib/Target/AArch64/AArch64LegalizerInfo.cpp:518-527
+  // HACK: Don't allow faddp/addp for now. We don't pass down the type info
+  // necessary to get this right today.
+  //
+  // It looks like addp/faddp is the only intrinsic that's impacted by this.
+  // All other intrinsics fully describe the required types in their names.
+  //
+  // (See: https://bugs.llvm.org/show_bug.cgi?id=40968)
----------------
aemerson wrote:
> kristof.beyls wrote:
> > Over time, more intrinsics get added.
> > So, making an exception based on a specific intrinsic name here seems like it's a time bomb waiting to go off when new intrinsics get added which have the same properties resulting in a silent codegen faults.
> > Wouldn't it be possible to check for the properties of the intrinsics that result in incorrect code generation and check on those properties rather than explicitly blacklist based on name?
> You're right, this is not a solution that fixes the real issue. It's a workaround that we plan to be in place until we settle on a way to fix this in the general case.
> 
> Unfortunately, barring disallowing all potentially int/fp type overloaded intrinsics during translation, we don't have a good way to detect these problematic ones automatically. This issue happens when no other attribute of the intrinsic call disambiguates the selected opcode, so element size etc isn't enough. Knowing that isn't generally possible unless we analyze all the selection patterns.
> 
> If you like, we could expand this fallback to something more general, up until falling back to all neon intrinsics. However I think that's probably unnecessary.
One way I can think of making this more general that //might// work is to check if both of the following hold:

1) The intrinsic is overloaded
2) There is at least one operand being passed into the intrinsic that is a vector/aggregate type

I think that both of these being true is sufficient to cover all such cases. Case (1) covers the fact that we have an intrinsic with more than one instruction associated with it in the first case.

Case (2) ought to work because for any scalar type, the register bank //ought to// cover for the missing type info. With vectors, the type info isn't implied by the register bank, and so that's where I think we're going wrong.


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

https://reviews.llvm.org/D59062