[PATCH] D154528: [AMDGPU][GlobalISel] Generate fast fp64-to-fp16 conversions in unsafe mode.

[Jay Foad via Phabricator via llvm-commits]

foad added inline comments.


================
Comment at: llvm/lib/CodeGen/SelectionDAG/DAGCombiner.cpp:17037-17040
+    // Avoid folding legal fp_rounds into non-legal ones.
+    if (LegalDAG &&
+        TLI.getOperationAction(ISD::FP_ROUND, VT) != TargetLowering::Legal)
+      return SDValue();
----------------
kosarev wrote:
> foad wrote:
> > kosarev wrote:
> > > arsenm wrote:
> > > > kosarev wrote:
> > > > > This doesn't seem to cause any test failures here upstream and eliminates the need for the True16 f16 = fp_round f64 pattern downstream.
> > > > > 
> > > > > Not sure if we want that in a separate patch or better keep here to provide some context. It looks problematic to give it a real test without having some True16 support.
> > > > > 
> > > > I think you're supposed to be checking LegalOperations, not LegalDAG. Use the hasOperation helper?
> > > For some reason that I don't quite understand `LegalOperations` gets raised as soon as vector operations are legalised:
> > > 
> > > ```
> > >   LegalOperations = Level >= AfterLegalizeVectorOps;
> > > ```
> > > 
> > > So relying on that flag would mean we forbid the fold during the `AfterLegalizeVectorOps` combine.
> > "So relying on that flag would mean we forbid the fold during the AfterLegalizeVectorOps combine." - why is that a problem?
> I'm thinking of cases where legalisation of vector operations may result in a chain of fp_rounds, which won't be combined before legalisation of non-vector operations because we forbid the fold too early.
LegalizeVectorOps mostly just scalarizes things, so it is unlikely to introduce fp_rounds unless there were already (vector typed) fp_rounds in the input, which could have been combined earlier.

So I don't think this will be a problem. I would go with `LegalOperations` unless/until you have a real example of a problem that we can look at.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D154528