[PATCH] D37051: Model cache size and associativity in TargetTransformInfo

[Tobias Grosser via Phabricator via llvm-commits]

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


================
Comment at: include/llvm/Analysis/TargetTransformInfoImpl.h:348
+    case TargetTransformInfo::CL_L2D:
+      return llvm::Optional<unsigned>();
+    }
----------------
fhahn wrote:
> grosser wrote:
> > asb wrote:
> > > Shouldn't this have something like:
> > > ````
> > > default:
> > >   llvm_unreachable("Unknown TargetTransformInfo::CacheLevel")
> > > ```
> > The switch above is fully covered, but I can add this as a form of defensive programming. Thinking about this, it might also be useful to used typed enums for the cache level. I will update the patch accordingly.
> I think using an enum class would be enough. AFAIK, compilers are pretty good at warning about missing cases in switch statements over enum classes. I think there is no need for default, as the compiler would catch missing patterns.
Right. Especially clang got very good at this and I think gcc also supports this meanwhile.

I did not add a default case on purpose, as this would prevent compiler warnings about uncovered cases. I added a unreachable _after_ the swich, just do document this code is never reached. Most compilers, likely will know this already, but older compilers might warn if they cannot prove the return is unreachable. We help them out here as well.


https://reviews.llvm.org/D37051