[PATCH] D50629: AMDGPU: Fix getInstSizeInBytes

Nicolai Hähnle via Phabricator via llvm-commits llvm-commits at lists.llvm.org
Thu Aug 23 01:10:17 PDT 2018


nhaehnle added inline comments.


================
Comment at: lib/Target/AMDGPU/AMDGPUMCInstLower.cpp:308-314
+    // Sanity-check getInstSizeInBytes on explicitly specified CPUs (it cannot
+    // work correctly for the generic CPU).
+    //
+    // The isPseudo check really shouldn't be here, but unfortunately there are
+    // some negative lit tests that depend on being able to continue through
+    // here even when pseudo instructions haven't been lowered.
+    if (!MI->isPseudo() && STI.isCPUStringValid(STI.getCPU())) {
----------------
arsenm wrote:
> nhaehnle wrote:
> > arsenm wrote:
> > > We don't actually have a generic CPU? We just pick a default?
> > No, we don't. That's precisely the problem I mentioned elsewhere at some point and I brought up the possibility of just deciding that running without specifying a CPU is unsupported.
> > 
> > If you don't specify a CPU (or the CPU is unknown), you get a set of inconsistent feature bits that doesn't correspond to any real CPU. The consequence is that all sorts of MachineInstrs are generated during CodeGen which aren't actually considered supported by MC, so entering the instruction encoding logic will fail.
> > 
> > I briefly looked into ways of fixing this, and it's of course be possible, but it'd require fixing a *lot* of the existing tests.
> It looks like I changed this in r310258. I thought this was still defaulting to kaveri for HSA and tahiti for anything else
We end up generating FLAT_LOAD_DWORD_ci without having FeatureCIInsts, for example.


Repository:
  rL LLVM

https://reviews.llvm.org/D50629





More information about the llvm-commits mailing list