[PATCH] D67631: Add AutoUpgrade function to add new address space datalayout string to existing datalayouts.

[Alexander Richardson via Phabricator via llvm-commits]

arichardson added inline comments.


================
Comment at: llvm/trunk/include/llvm/Target/TargetMachine.h:160
   /// check on the validity of this DataLayout.
-  virtual bool isCompatibleDataLayout(const DataLayout &Candidate) const {
+  bool isCompatibleDataLayout(const DataLayout &Candidate) const {
     return DL == Candidate;
----------------
rnk wrote:
> arichardson wrote:
> > Why was virtual removed here? We override this in the MIPS backend for our CHERI fork.
> > 
> > If the intention is to not allow targets to override this, then the documentation also needs to be adjusted.
> > However, I doubt avoiding the virtual function call gives any measurable performance speedup.
> > 
> I dug through the history trying to figure out why it was made virtual, and wasn't able to find any strong reason for it. Obviously, no in tree targets use it. If you want to add it back, I'd approve such a patch.
> 
> Also, I seem to recall that overriding this wasn't actually enough for our use cases anyway, so it didn't seem that useful.
Looking through our history I was not able to see why we added the override. It was added during a merge from upstream with no explanation. I just removed the override and all tests still pass so it no longer appears necessary. Sorry for the noise.

Although I guess the `This hook provides a target specific check on the validity of this DataLayout.` comment should probably be deleted.


Repository:
  rL LLVM

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

https://reviews.llvm.org/D67631