[PATCH] D72001: CodeGen: Use LLT instead of EVT in getRegisterByName

[James Y Knight via Phabricator via llvm-commits]

jyknight added inline comments.


================
Comment at: llvm/lib/CodeGen/SelectionDAG/SelectionDAGISel.cpp:2255-2257
+  EVT VT = Op->getValueType(0);
+  LLT Ty = getLLTForType(*VT.getTypeForEVT(*CurDAG->getContext()),
+                         CurDAG->getDataLayout());
----------------
arsenm wrote:
> arsenm wrote:
> > jyknight wrote:
> > > It feels weird to have to go up through IR Type to convert between two lower-level kinds of type representation. Shouldn't there be a function to go directly EVT->LLT?
> > Well, the desired end state is for EVT to not exist so I don't really want to put in the (somewhat small) work to write a proper conversion function. I could maybe stick this in a wrapper somewhere. It feels like a layering violation anywhere I could put it, but I suppose putting it in LowLevelType.h would work.
> Another problem is you can't really accurately go back from EVT to LLT, as EVT discards pointerness. This isn't solving the problem here, but the lack of a convenient conversion forces you to think about this
Well, it didn't force me to think about it...how would I have recognized that as a potential problem by reading the series of calls `getLLTForType(getTypeForEVT(...))`?

It would be more obvious if the nonexistent function "getLLTForEVT" had a docstring noting that it cannot return a pointer LLT, since EVT doesn't represent those, and that if you might need one, you'll need to do something else.

(here it's of course fine, since this function can only be called with integral types.)


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

https://reviews.llvm.org/D72001