This is an archive of the discontinued LLVM Phabricator instance.

[GlobalISel][CallLowering] Use hasRetAttr for return flags on CallBases
ClosedPublic

Authored by paquette on Aug 19 2020, 9:52 AM.

Details

Summary

Given something like this:

declare signext i16 @signext_callee()
define i32 @caller() {
%res = call i16 @signext_callee()
}

CallLowering would miss that signext_callee's return value is sign extended, because it isn't on the call.

Use hasRetAttr on the CallBase to allow us to catch this.

Diff Detail

Event Timeline

paquette created this revision.Aug 19 2020, 9:52 AM
Herald added a project: Restricted Project. · View Herald TranscriptAug 19 2020, 9:52 AM
paquette requested review of this revision.Aug 19 2020, 9:52 AM

The change itself is fine and more consistent, but this is just making an existing problem worse. Instead of treating signext/zeroext as optimization hints on arguments and call results, we're actually introducing code to do the extension which we were promised was already done. We really should be emitting the equivalent of ISD::AssertZext/AssertSext

llvm/test/CodeGen/AArch64/GlobalISel/call-translator.ll
392

This doesn't actually need a G_ZEXT. In this context it's supposed to be an optimization hint, so really this should be something like

; CHECK:   [[COPY:%[0-9]+]]:_(s32) = COPY $w0
; CHECK: [[ZEXT_HINT]]:_(s32) = G_ASSERT_ZEXT [[COPY]]

The change itself is fine and more consistent, but this is just making an existing problem worse. Instead of treating signext/zeroext as optimization hints on arguments and call results, we're actually introducing code to do the extension which we were promised was already done. We really should be emitting the equivalent of ISD::AssertZext/AssertSext

Yeah, I'm actually working on that right now. The problem is that right now, in, say, this case:

declare signext i16 @signext_callee()
define i32 @foo() {
  %res = call i16 @signext_callee()
  %ext = sext i16 %res to i32
  ret i32 %ext
}

We wouldn't even know that we could emit a G_ASSERT_SEXT. All this patch is supposed to do is make sure that we get the attribute plumbing correct.

aemerson accepted this revision.Aug 19 2020, 10:20 AM

Agreed that adding G_ASSERT_{ZEXT,SEXT} would be useful. We can teach known bits about them and eliminate superfluous extends in the prelegalizer combiner.

This revision is now accepted and ready to land.Aug 19 2020, 10:20 AM
Herald added a project: Restricted Project. · View Herald TranscriptSep 28 2022, 7:38 PM