[PATCH] D113107: Support of expression granularity for _Float16.

[John McCall via Phabricator via llvm-commits]

rjmccall added inline comments.


================
Comment at: clang/lib/CodeGen/CGExprComplex.cpp:934
+  return ComplexExprEmitter(*this).EmitPromoted(E, DstTy);
+}
+
----------------
rjmccall wrote:
> `EmitPromotedComplexExpr` should look like `EmitPromotedScalarExpr`, i.e. it should start with an expression, check for the arithmetic cases where we directly support promoted emission, and otherwise emit the unpromoted pair and then promote it.
Oh, I think I see how we're talking past each other.  I keep telling you to make this look like the scalar EmitPromoted, and now it does — but unfortunately the scalar path is no longer correct, when I think it used to be.  Let me go suggest what the scalar path should look like.


================
Comment at: clang/test/CodeGen/X86/Float16-arithmetic.c:37
+  // CHECK: fptrunc float {{.*}} to half
+  // CHECK: ret half
+  return a + b + c;
----------------
Okay, you can't write tests like this.  By using wildcards everywhere, you've made this test so permissive that it failed to recognize that you had regressed the basic truncation avoidance that's the whole point of this patch.  You need to write this like so:

```
  CHECK: [[A:%.*]] = alloca half
  CHECK: [[B:%.*]] = alloca half
  ...
  CHECK: [[A_LOAD:%.*]] = load half, ptr [[A]],
  CHECK: [[A_EXT:%.*]] = fpext half [[A_LOAD]] to float
  CHECK: [[B_LOAD:%.*]] = load half, ptr [[B]],
  CHECK: [[B_EXT:%.*]] = fpext half [[B_LOAD]] to float
  CHECK: [[AB_ADD:%.*]] = fadd float [[A_EXT]], [[B_EXT]]
  CHECK: [[C_LOAD:%.*]] = load half, ptr [[C]],
  CHECK: [[C_EXT:%.*]] = fpext half [[C_LOAD]] to float
  CHECK: [[ABC_ADD:%.*]] = fadd float [[AB_ADD]], [[C_EXT]]
```

And consider using `CHECK-NEXT` liberally as soon as you're out of the function prologue and into the expression emission.

*All* of these tests need to be rewritten this way.


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

https://reviews.llvm.org/D113107