Page MenuHomePhabricator

[RISCV] Add support for local PIC addressing
AbandonedPublic

Authored by rogfer01 on Aug 13 2018, 6:46 AM.

Details

Summary

This is another small step towards adding PIC addressing support to the RISC-V backend.

In this change we enable the codegen flow to emit PC-relative adressing to (dso-)local variables (i.e. PIC addressing that does not involve the GOT table).

Accesses via GOT will be implemented in a future change once we have the missing MC bits needed.

The approach taken is close to the one used in other backends:

  • Wrap the address in a target specific DAG node. In this case RISCVISD::WRAPPER_PIC
  • Use as operand of the wrapper node the corresponding Target* dag node with a flag. This flag indicates the kind of PIC addressing (PC-rel or GOT).
  • Using a custom inserter we expand RISCVISD::WRAPPER_PIC using the usual sequence auipc + addi in a new basic block. We fold, if possible, the addi offset into its user load and store instructions.

The new basic block needs its label always be emitted. Currently AsmPrinter does not emit a label for a basic block if the control flow just falls through to the block. So this change adds a setLabelMustBeEmitted to MachineBasicBlock.

Diff Detail

Event Timeline

rogfer01 created this revision.Aug 13 2018, 6:46 AM
rogfer01 retitled this revision from [RISCV] Add support for local PIC accessing to [RISCV] Add support for local PIC addressing.Aug 13 2018, 6:46 AM
rogfer01 added inline comments.Aug 13 2018, 6:49 AM
lib/Target/RISCV/RISCVISelLowering.cpp
372–374

I don't have a test for this case yet. I have been unable get SelectionDAGBuiler to generate a constantpool from LLVM IR.

Suggestions?

efriedma added inline comments.Aug 13 2018, 12:09 PM
lib/Target/RISCV/RISCVISelLowering.cpp
372–374

Try a floating-point constant?

rogfer01 updated this revision to Diff 160563.Aug 14 2018, 5:52 AM

ChangeLog

  • Add test for ConstantPools
rogfer01 added inline comments.Aug 14 2018, 5:58 AM
lib/Target/RISCV/RISCVISelLowering.cpp
372–374

Thanks a lot @efriedma

Indeed a floating point constant helped, I just had to try harder and come up with a less naive testcase.

387

I'm not sure how ExternalSymbol must be dealt. Like GlobalAddress or they should always use the GOT?

efriedma added inline comments.Aug 14 2018, 12:28 PM
lib/Target/RISCV/RISCVISelLowering.cpp
387

An ExternalSymbol is basically a global which doesn't have a corresponding IR symbol, so treat it the same way as any other extern global. They're mostly used for runtime library calls (memcpy, soft-float math, etc.).

That said, I'm not sure you can actually trigger this codepath given that LowerCall lowers the callee of calls itself.

rogfer01 added inline comments.Aug 16 2018, 1:36 AM
lib/Target/RISCV/RISCVISelLowering.cpp
387

Certainly. On a closer look, this function is not called anywhere (it is private and is not a virtual override) so looks to me as dead code and I think it can be removed.

kito-cheng added inline comments.Aug 29 2018, 2:21 AM
lib/Target/RISCV/RISCVISelLowering.cpp
583

I am worry about that will become kind of barrier for local optimizations, such as instruction scheduling and Machine InstCombiner.

Those optimizations are only process one basic block at once, so it might block some optimization opportunity.

rogfer01 added inline comments.Aug 29 2018, 9:06 AM
lib/Target/RISCV/RISCVISelLowering.cpp
583

Good point.

Alternatively we can expand the PseudoLLA in AsmPrinter. I'm afraid it defeats a bit the original vision of being able to intersperse instructions between the AUIPC and the ADDI (perhaps I'm incorrectly presuming that not much can happen after AsmPrinter).

What do you think?

If I'm understanding correctly, the problem here is that the lui has to have the address of the auipc as a parameter.

Instead of using the basic block's label to compute the address of the auipc, could you assign a unique ID to each auipc, and convert that ID to a label in the AsmPrinter? That seems more robust dealing with MI optimizations.

sabuasal added inline comments.Aug 29 2018, 12:19 PM
lib/Target/RISCV/RISCVISelLowering.cpp
609

We already have a peephole optimization, and a machine function pass (D47857), that do this. are you sure we need this?

If I'm understanding correctly, the problem here is that the lui has to have the address of the auipc as a parameter.

Yep, exactly that.

Instead of using the basic block's label to compute the address of the auipc, could you assign a unique ID to each auipc, and convert that ID to a label in the AsmPrinter? That seems more robust dealing with MI optimizations.

OK, let me see what I come up with.

Thanks @efriedma !

lib/Target/RISCV/RISCVISelLowering.cpp
609

Right, perhaps we don't. I will look into it. Thanks for the pointer!

rogfer01 planned changes to this revision.Nov 7 2018, 12:36 PM

On hold for now.

rogfer01 abandoned this revision.Aug 7 2019, 12:14 AM

Not relevant anymore.