[NFC] Use new version of SelectionDAG::getLoad in NVPTX LowerFormalArguments

[gchatelet]

Summary

I've stumbled upon this code while trying to remove the following deprecated function

inline SDValue SelectionDAG::getLoad(EVT VT, const SDLoc &dl, SDValue Chain, SDValue Ptr,
          MachinePointerInfo PtrInfo, unsigned Alignment,
          MachineMemOperand::Flags MMOFlags = MachineMemOperand::MONone,
          const AAMDNodes &AAInfo = AAMDNodes(),
          const MDNode *Ranges = nullptr);

I don't know if the current behavior is expected or not so the fix may change.

[gchatelet]

In particular it seems dubious to use a boolean type to convey alignment. Was this intended ?
I tracked it up to https://reviews.llvm.org/D30011 so I've added @tra and @jlebar as potential reviewers, let me know if someone is a better fit for this review.

[gchatelet]

rebase

[tra]

let me know if someone is a better fit for this review.

You've reached the right audience. :-)

I don't know if the current behavior is expected or not so the fix may change.

The original code looks OK to me. If the aggregate is packed, we request alignment of 1. If the bool aggregateIsPacked is true, it will undergo implicit integer conversion to 1, otherwise, we don't specify the alignment (0). AFAICT it should work exactly the same way before and after your patch and call exactly the same overload with exactly the same values. It's possible I'm missing something, but I do not see it at the moment.

It would help if you could provide more details about the problem you're trying to solve. A simple reproduced on godbolt.org would be very helpful.

Can you add a test that would demonstrate the issue with the current code and how the results would look like after the fix?

As things stand, I can't say I understand what exactly is the problem you want to fix, nor how the patch changes anything.

[gchatelet]

Thx for the response!
As of this revision the patch is absolutely useless, you're right. I wanted to be sure of the intended semantic before switching this call to the new getLoad function.

AFAIR there is a risk for a bool value to be true but hold a value that is not 1. Also it felt suspicious to pass a bool in lieu of an unsigned so I just wanted to be absolutely sure it was not a mistake.

I'll upload the real patch shortly.

[gchatelet]

• Transition to new API

[gchatelet]

I renamed the patch so it better matches the intent.

[tra]

In D141830#4064547, @gchatelet wrote:

AFAIR there is a risk for a bool value to be true but hold a value that is not 1.

I do not think so. Integer promotion rules guarantee that 'true' converts to 1.

> §4.7/4 from the C++ 11 or 14 Standard, §7.8/4 from the C++ 17 Standard, §7.3.9/2 from the 20 Standard says (Integral Conversion)
> If the source type is bool, the value false is converted to zero and the value true is converted to one.

Also it felt suspicious to pass a bool in lieu of an unsigned so I just wanted to be absolutely sure it was not a mistake.

The last revision of the patch just explicitly does what the DAG.getLoad() oveload we've picked before did under the hood. We've intentionally provided an overload which acceps unsigned value for alignment. I do not see much of a point Creating Align() manually. For what it's worth your first patch variant was more readable.
Your diff still makes no functional changes.

Please do provide a test case illustrating that there's a problem that needs fixing.

[gchatelet]

In D141830#4067189, @tra wrote:

The last revision of the patch just explicitly does what the DAG.getLoad() oveload we've picked before did under the hood. We've intentionally provided an overload which acceps unsigned value for alignment. I do not see much of a point Creating Align() manually. For what it's worth your first patch variant was more readable.
Your diff still makes no functional changes.

This patch is indeed NFC (as the updated title suggests). There is no problem to fix apart reducing the number of places where we use integer types to convey alignment (or lack thereof).
The Align and MaybeAligntypes convey the intent better (I originally thought that the bool promotion was a mistake). Align also guarantees that the alignment is a valid power of two and represents it tersely (one byte).
You can see the original RFC to see why this can be beneficial.

If you feel strongly about it I can abandon this patch but IMHO being more explicit doesn't hurt and tremendously helps with subsequent refactoring.

[tra]

In D141830#4068313, @gchatelet wrote:

You can see the original RFC to see why this can be beneficial.

Ah. Now the patch does make sense. Sorry about the push back.

llvm/lib/Target/NVPTX/NVPTXISelLowering.cpp

2751–2752

I'd just fold it into the call itself.

SDValue P = DAG.getLoad(VecVT, dl, Root, VecAddr,
                                  MachinePointerInfo(srcValue), MaybeAlign(aggregateIsPacked ? 1 : 0),

[gchatelet]

In D141830#4069631, @tra wrote:

In D141830#4068313, @gchatelet wrote:

You can see the original RFC to see why this can be beneficial.

Ah. Now the patch does make sense. Sorry about the push back.

No worries, I should have been clearer on the motivation of this patch from the very beginning.
Thx for the review!

[gchatelet]

• Fold declaration into function call

llvm/lib/Target/NVPTX/NVPTXISelLowering.cpp

if (!PAL.hasParamAttr(i, Attribute::ByVal)) {
             LoadVT = MVT::i32;
 
           EVT VecVT = EVT::getVectorVT(F->getContext(), LoadVT, NumElts);
           SDValue VecAddr =
               DAG.getNode(ISD::ADD, dl, PtrVT, Arg,
                           DAG.getConstant(Offsets[VecIdx], dl, PtrVT));
           Value *srcValue = Constant::getNullValue(PointerType::get(
               EltVT.getTypeForEVT(F->getContext()), ADDRESS_SPACE_PARAM));
-          SDValue P =
-              DAG.getLoad(VecVT, dl, Root, VecAddr,
-                          MachinePointerInfo(srcValue), aggregateIsPacked,
+          SDValue P = DAG.getLoad(VecVT, dl, Root, VecAddr,
+                                  MachinePointerInfo(srcValue),
+                                  MaybeAlign(aggregateIsPacked ? 1 : 0),
                                   MachineMemOperand::MODereferenceable |
                                       MachineMemOperand::MOInvariant);
           if (P.getNode())
             P.getNode()->setIROrder(idx + 1);
           for (unsigned j = 0; j < NumElts; ++j) {
             SDValue Elt = DAG.getNode(ISD::EXTRACT_VECTOR_ELT, dl, LoadVT, P,
                                       DAG.getIntPtrConstant(j, dl));
             // We've loaded i1 as an i8 and now must truncate it back to i1
             if (EltVT == MVT::i1)
               Elt = DAG.getNode(ISD::TRUNCATE, dl, MVT::i1, Elt);