Page MenuHomePhabricator

[NVPTX] Fix a segfault for bitcasted calls with byval params
ClosedPublic

Authored by ldrumm on Sep 23 2022, 9:25 AM.

Details

Summary
`getFunctionParamOptimizedAlign` was being passed a null function
  argument when getting the callee of a bitcasted function symbol. This is
  because `CallBase::getCalledFunction` does not look through bitcasts.
  
  There is already code to handle this case in
  `NVPTXTargetLowering::getArgumentAlignment`, which is now hoisted into
  an NVPTX util.
  
  The alignment computation now gracefully handles computing alignment of
  virtual functions with a check for null.

Diff Detail

Repository
rG LLVM Github Monorepo
Build Status
Buildable 188430

Event Timeline

ldrumm created this revision.Sep 23 2022, 9:25 AM
ldrumm requested review of this revision.Sep 23 2022, 9:25 AM
Herald added a project: Restricted Project. · View Herald TranscriptSep 23 2022, 9:25 AM
tra accepted this revision.Sep 23 2022, 11:24 AM

LGTM with few nits.

llvm/lib/Target/NVPTX/NVPTXISelLowering.cpp
1429

Would we ever have an instruction other than Call which would also carry explicit alignment metadata?

4329

Nit: if (!(F && F->hasLocalLinkage())) would be a bit easier to understand, IMO.

llvm/lib/Target/NVPTX/NVPTXUtilities.cpp
328
This revision is now accepted and ready to land.Sep 23 2022, 11:24 AM
ldrumm updated this revision to Diff 462877.Sep 26 2022, 5:58 AM
ldrumm marked an inline comment as done.

Code review suggestions

ldrumm marked an inline comment as done and an inline comment as not done.Sep 26 2022, 5:59 AM
ldrumm added inline comments.
llvm/lib/Target/NVPTX/NVPTXISelLowering.cpp
1429

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 ?

llvm/lib/Target/NVPTX/NVPTXUtilities.cpp
328

Yes. I actually prefer that style too. Too many competing style guides in my head!

tra added inline comments.Sep 26 2022, 11:09 AM
llvm/lib/Target/NVPTX/NVPTXISelLowering.cpp
1429

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.

nikic added a subscriber: nikic.Sep 26 2022, 11:47 AM
nikic added inline comments.
llvm/lib/Target/NVPTX/NVPTXUtilities.cpp
349

The whole function can be reduced to dyn_cast<Function>(CB->getCalledOperand()->stripPointerCasts()).

ldrumm updated this revision to Diff 466799.Oct 11 2022, 6:58 AM
ldrumm marked an inline comment as done.
ldrumm added inline comments.
llvm/lib/Target/NVPTX/NVPTXISelLowering.cpp
1429

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.

llvm/lib/Target/NVPTX/NVPTXUtilities.cpp
349

Thanks. TIL about stripPointerCasts

ldrumm closed this revision.Oct 18 2022, 6:44 AM

Merged in https://reviews.llvm.org/rG940fa35ece5294a115a2fdba89ef6c095d90df0f wherein I forgot to note the differential revision