[PATCH] D81704: [WebAssembly] Adding 64-bit version of R_WASM_MEMORY_ADDR_* relocs

Mon Jul 6 17:58:35 PDT 2020

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

================
Comment at: llvm/lib/Target/WebAssembly/TargetInfo/WebAssemblyTargetInfo.cpp:40
+#define GET_INSTRINFO_ENUM 1
+#include "WebAssemblyGenInstrInfo.inc"
----------------
thakis wrote:
> aardappel wrote:
> > aardappel wrote:
> > > thakis wrote:
> > > > This is a bit awkward. It makes WebAssembly the only target that has its TargetInfo dir depend on a llvm-tblgen generated file. It doesn't cause actual issues, but it's irregular.
> > > > 
> > > > Make of this what you will :)
> > > I'd be happy to move this something else.. we just don't have any other .cpp that is shared between CodeGen and MC. I could add a new lib that both depend on? @dschuff 
> > Actually, the cleaner solution would be to have tablegen (or whatever creates the instruction mappings) not stick both generated functions in the same #ifdef, which is the root cause of this having to sit in a shared location in the first place. Or make the functions static. But that potentially affects other targets, etc.
> This came back to bite me today (or, well, actually, a few weeks ago, but I didn't notice until today): In the GN build (which isn't supported and which you don't have to care about; it's just FYI, nothing to act on), the .inc files get generated in the Target sub directory they best fit into. Due to this change, I had moved WebAssemblyGenInstrInfo.inc from WebAssembly/MCTargetDesc to WebAssembly/TargetInfo. I didn't realize that this had the effect that I now had a correct WebAssemblyGenInstrInfo.inc and a stale WebAssemblyGenInstrInfo.inc in different directories in build dirs on the bots, and they happened to pick up the stale one. Since the .inc include is unqualified, a wasm .td change today broke my bots. The hack fix was to delete the stale .inc file.
> 
> The cmake build puts all target llvm-tblgen output in lib/Target/$targetname so it's not a problem there (the GN build should do that too), but I thought it's a nice example of how "this looks a bit funny" turned into an actual problem down the road.
> 
> Again, nothing for you to do here :)
Sorry about that.. I wouldn't mind fixing this, question is what is the best fix? Do you know of a way to have tablegen generate one file per instruction mapping?

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D81704