[PATCH] D47592: [AArch64] Corrected FP16 Intrinsic range checks in Clang + added Sema tests

Sjoerd Meijer via Phabricator via cfe-commits cfe-commits at lists.llvm.org
Mon Jun 4 09:07:03 PDT 2018


SjoerdMeijer accepted this revision.
SjoerdMeijer added a comment.
This revision is now accepted and ready to land.

I think this looks ok now, just some nits inline.

Can you please upload your diffs with more context next time?



================
Comment at: utils/TableGen/NeonEmitter.cpp:2166
+void NeonEmitter::genIntrinsicRangeCheckCode(raw_ostream &OS,
                                         SmallVectorImpl<Intrinsic *> &Defs) {
   OS << "#ifdef GET_NEON_IMMEDIATE_CHECK\n";
----------------
Nit: can you realign this?


================
Comment at: utils/TableGen/NeonEmitter.cpp:2193
+	  if (Def->getBaseType().getElementSizeInBits() == 16 ||
+		  Def->getName().find('h') != std::string::npos)
+		// VCVTh operating on FP16 intrinsics in range [1, 16)
----------------
Nit: for a moment I thought this could match more cases than intended, but we have already checked for isVCVT_N, so should be fine?


================
Comment at: utils/TableGen/NeonEmitter.cpp:2194
+		  Def->getName().find('h') != std::string::npos)
+		// VCVTh operating on FP16 intrinsics in range [1, 16)
+		UpperBound = "15";
----------------
Nit: think you're (almost) repeating the comment above, so you can omit this one?


https://reviews.llvm.org/D47592





More information about the cfe-commits mailing list