[PATCH] D134548: [NVPTX] Fix a segfault for bitcasted calls with byval params

Tue Oct 11 06:58:45 PDT 2022

ldrumm added inline comments.

================
Comment at: llvm/lib/Target/NVPTX/NVPTXISelLowering.cpp:1429
       // Check if we have call alignment metadata
       if (getAlign(*CI, Idx, Alignment))
         return Align(Alignment);
----------------
tra wrote:
> ldrumm wrote:
> > tra wrote:
> > > Would we ever have an instruction other than `Call` which would also carry explicit alignment metadata? 
> > allocas, loads, and stores, though I'm not sure if that's what you're asking. Could you expand a little on what you mean? The code here is only dealing with argument alignment so in isolation the answer is "no", but I have a feeling you might be hinting at something deeper ?
> If `CallBase` can be an instruction other than `Call`, will we be able to get argument alignment via `getAlign()` and if so, should we?
> 
> Another observation here is that previously digging through bitcasts was done under `if ( dyn_cast<CallInst>(CB))` while now it's done for any `CB` instruction. It looks like it should work (getMaybeBitcastedCallee will bail out on other instructions, if it can't get ), but was that done that intentionally? If looking through bitcasts is applicable to the `Call` instruction only, I'd prefer to keep it under the `if` to make it obvious.
I don't think this can ever be anything other than a call. It's within `if (const auto *CI = dyn_cast<CallInst>(CB)) {`. The getMaybeBitcastedCallee is indeed used in more possible places but I figured this to be safe in the current usage.

================
Comment at: llvm/lib/Target/NVPTX/NVPTXUtilities.cpp:349
+  // Function?
+  return dyn_cast<Function>(CalleeV);
+}
----------------
nikic wrote:
> The whole function can be reduced to `dyn_cast<Function>(CB->getCalledOperand()->stripPointerCasts())`.
Thanks. TIL about `stripPointerCasts`

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

https://reviews.llvm.org/D134548