[PATCH] D32684: [X86] Adding new LLVM TableGen backend that generates the X86 backend memory folding tables.

Ayman Musa via Phabricator via llvm-commits llvm-commits at lists.llvm.org
Sun May 14 01:04:03 PDT 2017


aymanmus added inline comments.


================
Comment at: utils/TableGen/X86FoldTablesEmitter.cpp:76
+// Do not add these instructions to any of the folding tables.
+const std::vector<const char *> NoFoldSet = {
+    "TCRETURNri64",
----------------
craig.topper wrote:
> Can this be an array now instead of a vector?
We only want to search this list for a match, so I think defining a vector and using the llvm::find function is more suitable.


================
Comment at: utils/TableGen/X86FoldTablesEmitter.cpp:257
+  for (unsigned i = 0, e = B->getNumBits(); i != e; ++i) {
+    if (BitInit *Bit = cast<BitInit>(B->getBit(i)))
+      Value |= uint64_t(Bit->getValue()) << i;
----------------
RKSimon wrote:
> Shouldn't this be dyn_cast if you want it to fail gracefully? Otherwise possibly replace that PrintFatalError with an assertion at the start of the loop?
It was a dynamic cast, but then @filcab noted that we can't expect getBit() to return something that is not a BitInit.
I personally agree with him.


================
Comment at: utils/TableGen/X86FoldTablesEmitter.cpp:268
+  if (B1->getNumBits() != B2->getNumBits())
+    PrintFatalError("Comparing two BitsInits with different sizes!");
+
----------------
RKSimon wrote:
> Are we better off with an assertion?
In this context I think this behavior is more suitable. Here we always try to compare the same fields of two different records, where the types of the fields must be the same.
Trying to compare two BitsInit with different sizes (thus different types) should alert that something wrong is happening.


================
Comment at: utils/TableGen/X86FoldTablesEmitter.cpp:588
+  // added to Table2Addr.
+  if (hasDefInList(MemRec, "SchedRW", "WriteRMW") && MemOutSize != RegOutSize &&
+      MemInSize == RegInSize) {
----------------
craig.topper wrote:
> Is the best way we have to detect these instructions?
I think it is.


https://reviews.llvm.org/D32684





More information about the llvm-commits mailing list